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.

Sign up for GitHub

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

Jump to bottom

Returning UrlTree from CanActivate guard breaks browser's history and leads to an inconsistent state #43101

Closed
samuelfernandez opened this issue Aug 10, 2021 · 1 comment
Closed

Returning UrlTree from CanActivate guard breaks browser's history and leads to an inconsistent state #43101

samuelfernandez opened this issue Aug 10, 2021 · 1 comment
Labels
area: router P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent router: guards/resolvers router: navigation pipe events/scheduleNavigation/transitions observable
Milestone
Backlog

Comments

@samuelfernandez
Copy link

Which @angular/* package(s) are the source of the bug?

router

Is this a regression?

No

Description

Returning a UrlTree from CanActivate guard should return in cancelling current navigation and redirecting to a new one.

The following events result in an inconsistent state:

  • User navigates through various routes
  • He performs an action, old routes won't be accessible because a CanActivate guard now returns a UrlTree to a valid route
  • User goes back with the browser. First time it works well and redirection to new route happens. Second time onwards the view is correct, but now the url is out of sync with current activated route .

Please provide a link to a minimal reproduction of the bug

https://stackblitz.com/edit/angular-router-issue?file=src/app/app.component.ts

Please provide the exception or error you saw

router.url !== window.location.pathname

Please provide the environment you discovered this bug in

Happens at least in v11 and in v12, I've tested with both environments

Anything else?

Demo of the issue:

routing-issue
@atscott atscott added area: router P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent router: guards/resolvers router: navigation pipe events/scheduleNavigation/transitions observable labels Aug 10, 2021
@ngbot ngbot bot added this to the Backlog milestone Aug 10, 2021
atscott added a commit to atscott/angular that referenced this issue Aug 10, 2021
@atscott
fix(router): eagerly update internal state on browser-triggered navig…
35d219e 
…ations

The management of `browserUrlTree` currently has several problems with
correctly tracking the actual state of the browser.

This change makes the Router eagerly update the `browserUrlTree` when
handling navigations triggered by browser events (i.e., not 'imperative'). This
is becauses with those types of navigations, the browser URL bar is
_already_ updated. If we do not update the internal tracking of the
`browserUrlTree`, we will be out of sync with the real URL if the
navigation is rejected.

It would be best if we could remove `browserUrlTree` completely, but doing that
would require a lot more investigation and is blocked by angular#27059 because
the SpyLocation used in tests does not emulate real browser behavior.

fixes angular#43101
@atscott atscott mentioned this issue Aug 10, 2021
Closed
atscott added a commit to atscott/angular that referenced this issue Aug 10, 2021
@atscott
fix(router): eagerly update internal state on browser-triggered navig…
ff32c85 
…ations

The management of `browserUrlTree` currently has several problems with
correctly tracking the actual state of the browser.

This change makes the Router eagerly update the `browserUrlTree` when
handling navigations triggered by browser events (i.e., not 'imperative'). This
is because with those types of navigations, the browser URL bar is
_already_ updated. If we do not update the internal tracking of the
`browserUrlTree`, we will be out of sync with the real URL if the
navigation is rejected.

It would be best if we could remove `browserUrlTree` completely, but doing that
would require a lot more investigation and is blocked by angular#27059 because
the SpyLocation used in tests does not emulate real browser behavior.

fixes angular#43101
dylhunn pushed a commit that referenced this issue Aug 16, 2021
@atscott @dylhunn
fix(router): eagerly update internal state on browser-triggered navig…
6449590 
…ations (#43102)

The management of `browserUrlTree` currently has several problems with
correctly tracking the actual state of the browser.

This change makes the Router eagerly update the `browserUrlTree` when
handling navigations triggered by browser events (i.e., not 'imperative'). This
is because with those types of navigations, the browser URL bar is
_already_ updated. If we do not update the internal tracking of the
`browserUrlTree`, we will be out of sync with the real URL if the
navigation is rejected.

It would be best if we could remove `browserUrlTree` completely, but doing that
would require a lot more investigation and is blocked by #27059 because
the SpyLocation used in tests does not emulate real browser behavior.

fixes #43101

PR Close #43102
@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 16, 2021
TeriGlover pushed a commit to TeriGlover/angular that referenced this issue Sep 16, 2021
@atscott @TeriGlover
fix(router): eagerly update internal state on browser-triggered navig…
af27441 
…ations (angular#43102)

The management of `browserUrlTree` currently has several problems with
correctly tracking the actual state of the browser.

This change makes the Router eagerly update the `browserUrlTree` when
handling navigations triggered by browser events (i.e., not 'imperative'). This
is because with those types of navigations, the browser URL bar is
_already_ updated. If we do not update the internal tracking of the
`browserUrlTree`, we will be out of sync with the real URL if the
navigation is rejected.

It would be best if we could remove `browserUrlTree` completely, but doing that
would require a lot more investigation and is blocked by angular#27059 because
the SpyLocation used in tests does not emulate real browser behavior.

fixes angular#43101

PR Close angular#43102
TeriGlover pushed a commit to TeriGlover/angular that referenced this issue Sep 22, 2021
@atscott @TeriGlover
fix(router): eagerly update internal state on browser-triggered navig…
cae2267 
…ations (angular#43102)

The management of `browserUrlTree` currently has several problems with
correctly tracking the actual state of the browser.

This change makes the Router eagerly update the `browserUrlTree` when
handling navigations triggered by browser events (i.e., not 'imperative'). This
is because with those types of navigations, the browser URL bar is
_already_ updated. If we do not update the internal tracking of the
`browserUrlTree`, we will be out of sync with the real URL if the
navigation is rejected.

It would be best if we could remove `browserUrlTree` completely, but doing that
would require a lot more investigation and is blocked by angular#27059 because
the SpyLocation used in tests does not emulate real browser behavior.

fixes angular#43101

PR Close angular#43102
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
area: router P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent router: guards/resolvers router: navigation pipe events/scheduleNavigation/transitions observable
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

fix(router): eagerly update internal state on browser-triggered navigations atscott/angular
2 participants
@atscott @samuelfernandez