Skip to content
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

Internal router transition Subject has stale state #30340

Closed
jasonaden opened this issue May 8, 2019 · 1 comment

Comments

Projects
None yet
1 participant
@jasonaden
Copy link
Contributor

commented May 8, 2019

Navigations are initialized by sending a new value into Router.transitions. In order to do so, we take the new value and merge it with the previous value. However, that previous value will always be the previous value prior to navigations having been run.

The real data we need is the NavigationTransition that most recently passed to the subscribe on Router.navigations. This will contain reference to the recent urlAfterRedirects and any other data added to the transition through the process of navigating.

Starting with old values can produce incorrect behavior. For instance, a test in Google navigates to a route, then navigates to the same route again, which in the router codebase falls into a path that essentially marks it to do nothing. Upon marking it to do nothing, we attempt to get the state right for the next navigation. However, the value is stale due to the above description.

The next navigation they are performing in the test is hitting back, which goes to the root. Because they happened to start on root, then navigate twice to the same place, then specifically go back to the root again, the back operation does nothing because the router thinks it’s already there.

@jasonaden

This comment has been minimized.

Copy link
Contributor Author

commented May 8, 2019

When fixed, need to un-revert this PR and verify this set of tests pass.

jasonaden added a commit to jasonaden/angular that referenced this issue May 8, 2019

fix(router): ensure navigations start with the current URL value inca…
…se redirect is skipped

In some cases where multiple navigations happen to the same URL, the router will not process a given URL. In those cases, we fall into logic that resets state for the next navigation. One piece of this resetting is to set the `browserUrlTree` to the most recent `urlAfterRedirects`i.

However, there was bug in this logic because in some cases the `urlAfterRedirects` is a stale value. This happens any time a URL won't be processed, and the previous URL will also not be processed. This creates unpredictable behavior, not the least of which ends up being a broken `back` button.

This PR kicks off new navigations with the current value the router assumes is in the browser. All the logic around how to handle future navigations is based on this value compared to the current transition, so it's important to kick off all new navigations with the current value so in the edge case described above we don't end up with an old value being set into `browserUrlTree`.

Fixes angular#30340
Related to angular#30160

jasonaden added a commit to jasonaden/angular that referenced this issue May 8, 2019

fix(router): ensure navigations start with the current URL value inca…
…se redirect is skipped

In some cases where multiple navigations happen to the same URL, the router will not process a given URL. In those cases, we fall into logic that resets state for the next navigation. One piece of this resetting is to set the `browserUrlTree` to the most recent `urlAfterRedirects`i.

However, there was bug in this logic because in some cases the `urlAfterRedirects` is a stale value. This happens any time a URL won't be processed, and the previous URL will also not be processed. This creates unpredictable behavior, not the least of which ends up being a broken `back` button.

This PR kicks off new navigations with the current value the router assumes is in the browser. All the logic around how to handle future navigations is based on this value compared to the current transition, so it's important to kick off all new navigations with the current value so in the edge case described above we don't end up with an old value being set into `browserUrlTree`.

Fixes angular#30340
Related to angular#30160

@alxhub alxhub closed this in 0fd9d08 May 15, 2019

alxhub added a commit that referenced this issue May 15, 2019

fix(router): ensure navigations start with the current URL value inca…
…se redirect is skipped (#30344)

In some cases where multiple navigations happen to the same URL, the router will not process a given URL. In those cases, we fall into logic that resets state for the next navigation. One piece of this resetting is to set the `browserUrlTree` to the most recent `urlAfterRedirects`i.

However, there was bug in this logic because in some cases the `urlAfterRedirects` is a stale value. This happens any time a URL won't be processed, and the previous URL will also not be processed. This creates unpredictable behavior, not the least of which ends up being a broken `back` button.

This PR kicks off new navigations with the current value the router assumes is in the browser. All the logic around how to handle future navigations is based on this value compared to the current transition, so it's important to kick off all new navigations with the current value so in the edge case described above we don't end up with an old value being set into `browserUrlTree`.

Fixes #30340
Related to #30160

PR Close #30344

BioPhoton added a commit to BioPhoton/angular that referenced this issue May 21, 2019

fix(router): ensure navigations start with the current URL value inca…
…se redirect is skipped (angular#30344)

In some cases where multiple navigations happen to the same URL, the router will not process a given URL. In those cases, we fall into logic that resets state for the next navigation. One piece of this resetting is to set the `browserUrlTree` to the most recent `urlAfterRedirects`i.

However, there was bug in this logic because in some cases the `urlAfterRedirects` is a stale value. This happens any time a URL won't be processed, and the previous URL will also not be processed. This creates unpredictable behavior, not the least of which ends up being a broken `back` button.

This PR kicks off new navigations with the current value the router assumes is in the browser. All the logic around how to handle future navigations is based on this value compared to the current transition, so it's important to kick off all new navigations with the current value so in the edge case described above we don't end up with an old value being set into `browserUrlTree`.

Fixes angular#30340
Related to angular#30160

PR Close angular#30344
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.