refactor(router): make recognize synchronous to expose it later by yelhouti · Pull Request #30410 · angular/angular

yelhouti · 2019-05-11T04:48:29Z

make recognize synchronous as suggested by @jasonaden in PR 15826.
This is better since it will allow to convert URLs to snapshot and use
metadata without the need for Observables

PR Checklist

Please check if your PR fulfills the following requirements:

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

Bugfix
Feature
Code style update (formatting, local variables)
Refactoring (no functional changes, no api changes)
Build related changes
CI related changes
Documentation content changes
angular.io application / infrastructure changes
Other... Please describe:

What is the current behavior?

recognize is asynchronous as explained by @jasonaden in PR #15826.

Issue Number: #15826

What is the new behavior?

recognize is synchronous, this will allow to convert URLs to snapshot and use
metadata without the need for Observables

Does this PR introduce a breaking change?

Yes
No

Other information

@jasonaden

make recognize synchronous as suggested by @jasonaden in PR 15826. This is better since it would allow to convert URLs to snapshot and use metadata without the need for Observables

googlebot · 2019-05-11T04:48:33Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

yelhouti · 2019-05-11T04:49:28Z

I signed it!

googlebot · 2019-05-11T04:49:38Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

yelhouti · 2019-05-17T01:13:24Z

@jasonaden I can close this, since as you can see in my other PR #30411 , even if recognize is synchronous it can't be used to retrieve a Snapshot without the use of follow redirect that is async.

atscott

Hi @yelhouti, can you rebase this PR? I'm generally in favor of removing the Observable nature from recognize since it's not necessary.

atscott · 2020-08-06T20:52:26Z

packages/router/src/recognize.ts

-      this.inheritParamsAndData(routeState._root);
-      return of (routeState);
-
-    } catch (e) {


Should this try...catch be moved to operators/recognize.ts to avoid unintentionally throwing errors that can't be handled by catchError?

You also removed the test for the error case below. Maybe add that back and use the recognize operator.

…th parents There are two parts to this commit: 1. Revert the changes from angular#38379. This change had an incomplete view of how things worked and also diverged the implementations of `applyRedirects` and `recognize` even more. 2. Apply the fixes from the `recognize` algorithm to ensure that named outlets with empty path parents can be matched. This change also passes all the tests that were added in angular#38379 with the added benefit of being a more complete fix that stays in-line with the `recognize` algorithm. This was made possible by using the same approach for `split` by always creating segments for empty path matches (previously, this was only done in `applyRedirects` if there was a `redirectTo` value). At the end of the expansions, we need to squash all empty segments so that serializing the final `UrlTree` returns the same result as before. Fixes angular#39952 Fixes angular#10726 Closes angular#30410

…th parents (angular#40029) There are two parts to this commit: 1. Revert the changes from angular#38379. This change had an incomplete view of how things worked and also diverged the implementations of `applyRedirects` and `recognize` even more. 2. Apply the fixes from the `recognize` algorithm to ensure that named outlets with empty path parents can be matched. This change also passes all the tests that were added in angular#38379 with the added benefit of being a more complete fix that stays in-line with the `recognize` algorithm. This was made possible by using the same approach for `split` by always creating segments for empty path matches (previously, this was only done in `applyRedirects` if there was a `redirectTo` value). At the end of the expansions, we need to squash all empty segments so that serializing the final `UrlTree` returns the same result as before. Fixes angular#39952 Fixes angular#10726 Closes angular#30410 PR Close angular#40029

…th parents (#40029) (#40315) There are two parts to this commit: 1. Revert the changes from #38379. This change had an incomplete view of how things worked and also diverged the implementations of `applyRedirects` and `recognize` even more. 2. Apply the fixes from the `recognize` algorithm to ensure that named outlets with empty path parents can be matched. This change also passes all the tests that were added in #38379 with the added benefit of being a more complete fix that stays in-line with the `recognize` algorithm. This was made possible by using the same approach for `split` by always creating segments for empty path matches (previously, this was only done in `applyRedirects` if there was a `redirectTo` value). At the end of the expansions, we need to squash all empty segments so that serializing the final `UrlTree` returns the same result as before. Fixes #39952 Fixes #10726 Closes #30410 PR Close #40029 PR Close #40315

angular-automatic-lock-bot · 2021-02-05T16:35:05Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

refactor(router): make recognize synchronous to expose it later

2070595

make recognize synchronous as suggested by @jasonaden in PR 15826. This is better since it would allow to convert URLs to snapshot and use metadata without the need for Observables

yelhouti requested a review from a team May 11, 2019 04:48

googlebot added the cla: no label May 11, 2019

googlebot added cla: yes and removed cla: no labels May 11, 2019

jasonaden added the area: router label May 15, 2019

ngbot bot added this to the needsTriage milestone May 15, 2019

atscott requested changes Aug 6, 2020

View reviewed changes

atscott added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Aug 6, 2020

josephperrott closed this in e43f7e2 Jan 5, 2021

angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(router): make recognize synchronous to expose it later#30410

refactor(router): make recognize synchronous to expose it later#30410
yelhouti wants to merge 1 commit intoangular:masterfrom
yelhouti:router-recognize

yelhouti commented May 11, 2019

Uh oh!

googlebot commented May 11, 2019

Uh oh!

yelhouti commented May 11, 2019

Uh oh!

googlebot commented May 11, 2019

Uh oh!

yelhouti commented May 17, 2019 •

edited

Loading

Uh oh!

atscott left a comment

Uh oh!

atscott Aug 6, 2020

Uh oh!

angular-automatic-lock-bot bot commented Feb 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

yelhouti commented May 11, 2019

PR Checklist

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

Other information

Uh oh!

googlebot commented May 11, 2019

What to do if you already signed the CLA

Individual signers

Corporate signers

Uh oh!

yelhouti commented May 11, 2019

Uh oh!

googlebot commented May 11, 2019

Uh oh!

yelhouti commented May 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

atscott left a comment

Choose a reason for hiding this comment

Uh oh!

atscott Aug 6, 2020

Choose a reason for hiding this comment

Uh oh!

angular-automatic-lock-bot bot commented Feb 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yelhouti commented May 17, 2019 •

edited

Loading