fix(router): fix navigation ignoring logic to compare to the browser url #37716
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
atscott
added
area: router
target: major
atscott
force-pushed
the
imperativenavtest-resubmit
branch
from
a72e895
to
2368c37
Compare
There was a problem hiding this comment.
Thanks for the fix @atscott, I've left a few minor comments. Please have a look when you get a chance.
atscott
force-pushed
the
imperativenavtest-resubmit
branch
from
2368c37
to
e0dfb84
Compare
This PR changes the logic for determining when to skip route processing from using the URL of the last attempted navigation to the actual resulting URL after that transition. Because guards may prevent navigation and reset the browser URL, the raw URL of the previous transition may not match the actual URL of the browser at the end of the navigation process. For that reason, we need to use `urlAfterRedirects` instead. Other notes: These checks in scheduleNavigation were added in angular@eb2ceff The test still passes and, more surprisingly, passes if the checks are removed completely. There have likely been changes to the navigation handling that handle the test in a different way. That said, it still appears to be important to keep the checks there in some capacity because it does affect how many navigation events occur. This addresses an issue that came up in angular#16710: angular#16710 (comment) This also partially addresses angular#13586 in fixing history for imperative navigations that are cancelled by guards.
atscott
force-pushed
the
imperativenavtest-resubmit
branch
from
e0dfb84
to
2d4d32d
Compare
|
@AndrewKushnir - Thanks for the review! I've addressed your comments - PTAL. |
AndrewKushnir
approved these changes
Jun 30, 2020
|
[global presubmit(https://test.corp.google.com/ui#id=OCL:318866503:BASE:319047837:1593570501085:2a82c23f) showing no failures. caretaker note: please merge and sync this one separately - it's a resubmit (with a fix) of a previous commit that caused a failure in g3 and had to be rolled back. |
|
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. |
profanis
pushed a commit
to profanis/angular
that referenced
this pull request
Sep 5, 2020
…url (angular#37716) This PR changes the logic for determining when to skip route processing from using the URL of the last attempted navigation to the actual resulting URL after that transition. Because guards may prevent navigation and reset the browser URL, the raw URL of the previous transition may not match the actual URL of the browser at the end of the navigation process. For that reason, we need to use `urlAfterRedirects` instead. Other notes: These checks in scheduleNavigation were added in angular@eb2ceff The test still passes and, more surprisingly, passes if the checks are removed completely. There have likely been changes to the navigation handling that handle the test in a different way. That said, it still appears to be important to keep the checks there in some capacity because it does affect how many navigation events occur. This addresses an issue that came up in angular#16710: angular#16710 (comment) This also partially addresses angular#13586 in fixing history for imperative navigations that are cancelled by guards. PR Close angular#37716
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR changes the logic for determining when to skip route processing from
using the URL of the last attempted navigation to the actual resulting URL after
that transition.
Because guards may prevent navigation and reset the browser URL, the raw
URL of the previous transition may not match the actual URL of the
browser at the end of the navigation process. For that reason, we need to use
urlAfterRedirectsif the previous navigation failed.Note that this is a resubmit of d3a8175 which caused a failure in Google. I've verified that this change does not result in the same failure but this PR should still be merged and synced separately.