Skip to content

fix(router): ensure navigations start with the current URL value inca… - #30344

Closed
jasonaden wants to merge 3 commits into
angular:masterfrom
jasonaden:fix_30340_stale_transition
Closed

jasonaden wants to merge 3 commits into
angular:masterfrom
jasonaden:fix_30340_stale_transition

Conversation

@jasonaden

Copy link
Copy Markdown
Contributor

…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 urlAfterRedirectsi.

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

@jasonaden
jasonaden requested a review from a team May 8, 2019 23:01
@jasonaden
jasonaden requested a review from alxhub May 8, 2019 23:01
@jasonaden jasonaden added the target: patch This PR is targeted for the next patch release label May 8, 2019
jasonaden added 2 commits May 8, 2019 16:03
…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
jasonaden force-pushed the fix_30340_stale_transition branch from 342806b to ec5c04a Compare May 8, 2019 23:09
@jasonaden

Copy link
Copy Markdown
Contributor Author

Presubmit

@alxhub alxhub closed this in 0fd9d08 May 15, 2019
alxhub pushed a commit that referenced this pull request May 15, 2019
alxhub pushed a commit that referenced this pull request May 15, 2019
…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
alxhub pushed a commit that referenced this pull request May 15, 2019
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
…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
BioPhoton pushed a commit to BioPhoton/angular that referenced this pull request May 21, 2019
@angular-automatic-lock-bot

Copy link
Copy Markdown

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.

@angular-automatic-lock-bot angular-automatic-lock-bot Bot locked and limited conversation to collaborators Sep 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

action: merge The PR is ready for merge by the caretaker cla: yes target: patch This PR is targeted for the next patch release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Internal router transition Subject has stale state

3 participants