fix(router): eagerly update internal state on browser-triggered navigations #43102
Conversation
This file contains hidden or 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
…ations The management of `browserUrlTree` currently has several problems with correctly tracking the actual state of the browser. This change makes the Router eagerly update the `browserUrlTree` when handling navigations triggered by browser events (i.e., not 'imperative'). This is because with those types of navigations, the browser URL bar is _already_ updated. If we do not update the internal tracking of the `browserUrlTree`, we will be out of sync with the real URL if the navigation is rejected. It would be best if we could remove `browserUrlTree` completely, but doing that would require a lot more investigation and is blocked by angular#27059 because the SpyLocation used in tests does not emulate real browser behavior. fixes angular#43101
35d219e to
ff32c85
Compare
|
global presubmit with reruns shows no failures due to this change. |
|
merge assistance: while global presubmit ran and was green, this CL should still be merged and synced on its own to be safe. |
|
@atscott FYI there are some CI failures which look legit, could you please have a look? (adding the "cleanup" label for now) |
2356261 to
0aa0e9a
Compare
|
If this must go in on its own, I will wait until tomorrow and do a second merge and sync. |
dylhunn
pushed a commit
that referenced
this pull request
Aug 16, 2021
…ations (#43102) The management of `browserUrlTree` currently has several problems with correctly tracking the actual state of the browser. This change makes the Router eagerly update the `browserUrlTree` when handling navigations triggered by browser events (i.e., not 'imperative'). This is because with those types of navigations, the browser URL bar is _already_ updated. If we do not update the internal tracking of the `browserUrlTree`, we will be out of sync with the real URL if the navigation is rejected. It would be best if we could remove `browserUrlTree` completely, but doing that would require a lot more investigation and is blocked by #27059 because the SpyLocation used in tests does not emulate real browser behavior. fixes #43101 PR Close #43102
atscott
added a commit
to atscott/angular
that referenced
this pull request
Sep 14, 2021
PR angular#43102 broke some use-cases. These tests document expected behavior and would have prevented angular#43446 and angular#43447. Recent changes have already addressed these issues, but it would still be a good idea to cover these use-cases in tests as well.
|
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. |
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.
The management of
browserUrlTreecurrently has several problems withcorrectly tracking the actual state of the browser.
This change makes the Router eagerly update the
browserUrlTreewhenhandling navigations triggered by browser events (i.e., not 'imperative'). This
is because with those types of navigations, the browser URL bar is
already updated. If we do not update the internal tracking of the
browserUrlTree, we will be out of sync with the real URL if thenavigation is rejected.
It would be best if we could remove
browserUrlTreecompletely, but doing thatwould require a lot more investigation and is blocked by #27059 because
the SpyLocation used in tests does not emulate real browser behavior.
fixes #43101