-
Notifications
You must be signed in to change notification settings - Fork 24.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor(router): make recognize synchronous to expose it later #30410
Conversation
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
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. What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
@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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @yelhouti, can you rebase this PR? I'm generally in favor of removing the Observable
nature from recognize
since it's not necessary.
this.inheritParamsAndData(routeState._root); | ||
return of (routeState); | ||
|
||
} catch (e) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 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 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 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 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 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 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 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 (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 (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
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
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:
PR Type
What kind of change does this PR introduce?
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?
Other information