Skip to content

fix(router): ensure URL is updated after second redirect with UrlUpda - #27680

Closed
jasonaden wants to merge 1 commit into
angular:7.2.xfrom
jasonaden:FW-46_early_url_cancellation_patch
Closed

jasonaden wants to merge 1 commit into
angular:7.2.xfrom
jasonaden:FW-46_early_url_cancellation_patch

Conversation

@jasonaden

Copy link
Copy Markdown
Contributor

teStrategy="eager"

Navigating to a route such as /users, you may get redirected to /login. Previously, if you go then route to /users again the URL will end up showing /users after the second redirect. This only happened in UrlUpdateStrategy="eager". This is now fixed so after the second redirect, the URL shows the correct page.

Patch version of #27523

@jasonaden
jasonaden changed the base branch from master to 7.1.x December 14, 2018 18:08
@jasonaden
jasonaden requested a review from mhevery December 14, 2018 18:09
@ngbot ngbot Bot added this to the needsTriage milestone Dec 14, 2018
@jasonaden jasonaden added action: merge The PR is ready for merge by the caretaker PR target: patch-only labels Dec 14, 2018
@googlebot

Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@mary-poppins

Copy link
Copy Markdown

You can preview 6771ed2 at https://pr27680-6771ed2.ngbuilds.io/.

@mhevery mhevery added cla: yes and removed cla: no labels Dec 14, 2018
@googlebot

Copy link
Copy Markdown

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@mhevery

mhevery commented Dec 14, 2018

Copy link
Copy Markdown
Contributor

@mhevery mhevery added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Dec 14, 2018
@mhevery
mhevery requested review from IgorMinar and removed request for mhevery December 19, 2018 22:34
@IgorMinar
IgorMinar requested review from alxhub and removed request for IgorMinar December 20, 2018 00:10
@jasonaden jasonaden added merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews labels Dec 20, 2018
@jasonaden
jasonaden force-pushed the FW-46_early_url_cancellation_patch branch from 6771ed2 to 95dc4b0 Compare December 20, 2018 22:19
@googlebot

Copy link
Copy Markdown

CLAs look good, thanks!

@mary-poppins

Copy link
Copy Markdown

You can preview 95dc4b0 at https://pr27680-95dc4b0.ngbuilds.io/.

…teStrategy="eager"

Navigating to a route such as `/users`, you may get redirected to `/login`. Previously, if you go then route to `/users` again the URL will end up showing `/users` after the second redirect. This only happened in `UrlUpdateStrategy="eager"`. This is now fixed so after the second redirect, the URL shows the correct page.

Fixes angular#27116
@jasonaden
jasonaden force-pushed the FW-46_early_url_cancellation_patch branch from 95dc4b0 to c01311f Compare December 21, 2018 22:23
@googlebot

Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@mary-poppins

Copy link
Copy Markdown

You can preview c01311f at https://pr27680-c01311f.ngbuilds.io/.

@jasonaden
jasonaden changed the base branch from 7.1.x to 7.2.x January 2, 2019 14:10
@kara kara added cla: yes and removed cla: no labels Jan 4, 2019
@googlebot

Copy link
Copy Markdown

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

@kara kara added action: review The PR is still awaiting reviews from at least one requested reviewer and removed action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note labels Jan 4, 2019
@kara kara assigned kara and alxhub and unassigned kara Jan 4, 2019
@kara

kara commented Jan 4, 2019

Copy link
Copy Markdown
Contributor

@jasonaden FYI moving out of merge queue because it still needs approval from @alxhub

@jasonaden jasonaden added action: merge The PR is ready for merge by the caretaker merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jan 14, 2019
@jasonaden

Copy link
Copy Markdown
Contributor Author

Caretaker, please merge this. It's just the patch version of #27523 created to help with resolving conflicts.

AndrewKushnir pushed a commit that referenced this pull request Jan 15, 2019
…teStrategy="eager" (#27680)

Navigating to a route such as `/users`, you may get redirected to `/login`. Previously, if you go then route to `/users` again the URL will end up showing `/users` after the second redirect. This only happened in `UrlUpdateStrategy="eager"`. This is now fixed so after the second redirect, the URL shows the correct page.

Fixes #27116

PR Close #27680
@AndrewKushnir

Copy link
Copy Markdown
Contributor

Merged this PR into patch branch using merge script.

AndrewKushnir pushed a commit that referenced this pull request Jan 16, 2019
…teStrategy="eager" (#27680)

Navigating to a route such as `/users`, you may get redirected to `/login`. Previously, if you go then route to `/users` again the URL will end up showing `/users` after the second redirect. This only happened in `UrlUpdateStrategy="eager"`. This is now fixed so after the second redirect, the URL shows the correct page.

Fixes #27116

PR Close #27680
jasonaden added a commit to jasonaden/angular that referenced this pull request Jan 22, 2019
With angular#27680, a bug was fixed where multiple redirects using `eager` URL update could cause navigation to fail. However, that fix introduced a problem where with `skipLocationChange` enabled, the URL tree rendered was not properly stored for reference. This specifically caused an issue with named router outlets and subsequent navigations not being recognized.

This PR stores the correct `UrlTree` for reference with later navigations. It fixes the regression introdued with angular#27680.

Fixes angular#28200
jasonaden added a commit to jasonaden/angular that referenced this pull request Jan 22, 2019
With angular#27680, a bug was fixed where multiple redirects using `eager` URL update could cause navigation to fail. However, that fix introduced a problem where with `skipLocationChange` enabled, the URL tree rendered was not properly stored for reference. This specifically caused an issue with named router outlets and subsequent navigations not being recognized.

This PR stores the correct `UrlTree` for reference with later navigations. It fixes the regression introdued with angular#27680.

Fixes angular#28200
alxhub pushed a commit that referenced this pull request Jan 22, 2019
With #27680, a bug was fixed where multiple redirects using `eager` URL update could cause navigation to fail. However, that fix introduced a problem where with `skipLocationChange` enabled, the URL tree rendered was not properly stored for reference. This specifically caused an issue with named router outlets and subsequent navigations not being recognized.

This PR stores the correct `UrlTree` for reference with later navigations. It fixes the regression introdued with #27680.

Fixes #28200

PR Close #28301
alxhub pushed a commit that referenced this pull request Jan 22, 2019
With #27680, a bug was fixed where multiple redirects using `eager` URL update could cause navigation to fail. However, that fix introduced a problem where with `skipLocationChange` enabled, the URL tree rendered was not properly stored for reference. This specifically caused an issue with named router outlets and subsequent navigations not being recognized.

This PR stores the correct `UrlTree` for reference with later navigations. It fixes the regression introdued with #27680.

Fixes #28200

PR Close #28300
vetom pushed a commit to vetom/angular that referenced this pull request Jan 31, 2019
With angular#27680, a bug was fixed where multiple redirects using `eager` URL update could cause navigation to fail. However, that fix introduced a problem where with `skipLocationChange` enabled, the URL tree rendered was not properly stored for reference. This specifically caused an issue with named router outlets and subsequent navigations not being recognized.

This PR stores the correct `UrlTree` for reference with later navigations. It fixes the regression introdued with angular#27680.

Fixes angular#28200

PR Close angular#28300
@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 14, 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 area: router cla: yes merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants