Skip to content

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

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

Closed

Conversation

jasonaden
Copy link
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

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

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

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
Copy link
Contributor

mhevery commented Dec 14, 2018

@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

CLAs look good, thanks!

@mary-poppins
Copy link

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

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

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

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
Copy link
Contributor

kara commented Jan 4, 2019

@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
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
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

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