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

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

Closed

Conversation

@jasonaden
Copy link
Contributor

jasonaden commented Dec 14, 2018

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 Dec 14, 2018

@jasonaden jasonaden requested a review from mhevery Dec 14, 2018

@ngbot ngbot bot added this to the needsTriage milestone Dec 14, 2018

@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 14, 2018

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.

@googlebot googlebot added the cla: no label Dec 14, 2018

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 14, 2018

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

@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 14, 2018

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

This comment has been minimized.

@mhevery mhevery requested review from IgorMinar and removed request for mhevery Dec 19, 2018

@IgorMinar IgorMinar requested review from alxhub and removed request for IgorMinar Dec 20, 2018

@jasonaden jasonaden force-pushed the jasonaden:FW-46_early_url_cancellation_patch branch from 6771ed2 to 95dc4b0 Dec 20, 2018

@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 20, 2018

CLAs look good, thanks!

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 20, 2018

fix(router): ensure URL is updated after second redirect with UrlUpda…
…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 #27116

@jasonaden jasonaden force-pushed the jasonaden:FW-46_early_url_cancellation_patch branch from 95dc4b0 to c01311f Dec 21, 2018

@googlebot

This comment has been minimized.

Copy link

googlebot commented Dec 21, 2018

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.

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

@mary-poppins

This comment has been minimized.

Copy link

mary-poppins commented Dec 21, 2018

@jasonaden jasonaden changed the base branch from 7.1.x to 7.2.x Jan 2, 2019

@benlesh

benlesh approved these changes Jan 3, 2019

@kara kara added cla: yes and removed cla: no labels Jan 4, 2019

@googlebot

This comment has been minimized.

Copy link

googlebot commented Jan 4, 2019

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

This comment has been minimized.

Copy link
Contributor

kara commented Jan 4, 2019

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

@jasonaden

This comment has been minimized.

Copy link
Contributor Author

jasonaden commented Jan 14, 2019

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

AndrewKushnir added a commit that referenced this pull request Jan 15, 2019

fix(router): ensure URL is updated after second redirect with UrlUpda…
…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

This comment has been minimized.

Copy link
Contributor

AndrewKushnir commented Jan 15, 2019

Merged this PR into patch branch using merge script.

AndrewKushnir added a commit that referenced this pull request Jan 16, 2019

fix(router): ensure URL is updated after second redirect with UrlUpda…
…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

fix(router): `skipLocationChange` with named outlets
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

fix(router): `skipLocationChange` with named outlets
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 added a commit that referenced this pull request Jan 22, 2019

fix(router): `skipLocationChange` with named outlets (#28301)
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 added a commit that referenced this pull request Jan 22, 2019

fix(router): `skipLocationChange` with named outlets (#28300)
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

ngfelixl added a commit to ngfelixl/angular that referenced this pull request Jan 28, 2019

fix(router): `skipLocationChange` with named outlets (angular#28300)
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

vetom added a commit to vetom/angular that referenced this pull request Jan 31, 2019

fix(router): `skipLocationChange` with named outlets (angular#28300)
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
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.