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

Deactivation Guard breaks the routing history #13586

Open
patrickracicot opened this Issue Dec 20, 2016 · 39 comments

Comments

Projects
None yet
@patrickracicot

patrickracicot commented Dec 20, 2016

I'm submitting a ... (check one with "x")

[X ] bug report => search github for a similar issue or PR before submitting
[ ] feature request
[ ] support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior
Currently, I have a deactivation guard on a route which will return false or true depending on a condition. To get to this guarded route, the user must pass though 3 navigation step. Now, once on the guarded route, when using location.back(), the guard is called. If it returns true, the previous route is loaded. If the guard returns false, the navigation is cancelled. But if we redo a Location.back() after a failed navigation, the previous route that will be loaded will be 2 steps in the history instead of 1 (user perception).

Workflow

Main    -- navigate to     --> Route 1
Route 1 -- navigate to     --> Route 2
Route 2 -- navigate to     --> Route 3
Route 3 -- location.back() --> guard returns true  --> Route 2
Route 2 -- navigate to     --> Route 3
Route 3 -- location.back() --> guard returns false --> Route 3
Route 3 -- location.back() --> guard returns true  --> Route 1  (should be Route 2)

Expected behavior
An expected behavior for a user would be that navigating back brings back to the previous routed page.
Workflow

Main    -- navigate to     --> Route 1
Route 1 -- navigate to     --> Route 2
Route 2 -- navigate to     --> Route 3
Route 3 -- location.back() --> guard returns true  --> Route 2
Route 2 -- navigate to     --> Route 3
Route 3 -- location.back() --> guard returns false --> Route 3
Route 3 -- location.back() --> guard returns true  --> Route 2  (expected)

Minimal reproduction of the problem with instructions
Plnkr

  1. Click button Nav to route1
  2. Click button Nav to route2
  3. Click button Nav to route3
  4. Click button Block Nav Back
  5. Click button Nav back
    • BOGUE: The location.back() routed on Route1 instead of Route2

Personnal investigation
After some investigation, I saw that in routerState$.then (router.ts line 752) this logic used when navigationIsSuccessful == false is pretty simply but it is the actual cause of this bug. Basically, when a deactivation guard is hit, the location of the browser is already changed to the previous route. Which means that when the guard returns false, the routerState$ runs his logic and calls resetUrlToCurrentUrlTree(). At this point we can see that we replace the state of the current location. But by doing this, we loose that route in the history which means that in my plunker, if we click the block nav back 3 times and then click the nav back we will actually kill the application.

What is the motivation / use case for changing the behavior?
This is for me a pretty big bug since a guard that returns false breaks alters the current routing history. In the case of our application this breaks the workflow and brings wrong business scopes to a user.

Please tell us about your environment:

Windows 10, NPM, Nodejs, Visual Studio 2015 (using nodejs for typescript compilation)

  • Angular version: 2.3.3

  • Browser: [ all ]

  • Language: [TypeScript 2.0.10 | ES5]

@saverett

This comment has been minimized.

Show comment
Hide comment
@saverett

saverett Jan 13, 2017

A Router.back() method wouldn't cover using the browser back and forward buttons though, correct? That seems to be the predominant use case.

saverett commented Jan 13, 2017

A Router.back() method wouldn't cover using the browser back and forward buttons though, correct? That seems to be the predominant use case.

@DzmitryShylovich

This comment has been minimized.

Show comment
Hide comment
@DzmitryShylovich

DzmitryShylovich Jan 13, 2017

Contributor

A Router.back() method wouldn't cover using the browser back and forward buttons though, correct?

yeah. So just returning history to the previous state is the only way I can think of here.

Contributor

DzmitryShylovich commented Jan 13, 2017

A Router.back() method wouldn't cover using the browser back and forward buttons though, correct?

yeah. So just returning history to the previous state is the only way I can think of here.

DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Jan 14, 2017

DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Jan 14, 2017

DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Jan 14, 2017

DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Feb 12, 2017

@michelherv

This comment has been minimized.

Show comment
Hide comment
@michelherv

michelherv Feb 22, 2017

Hi,
I have the same problem in case of Activation Guard. Have I to create a new issue ?

  • Angular version: 2.4.6
  • Browser: all
  • Language: TypeScript 2.1.5

michelherv commented Feb 22, 2017

Hi,
I have the same problem in case of Activation Guard. Have I to create a new issue ?

  • Angular version: 2.4.6
  • Browser: all
  • Language: TypeScript 2.1.5
@DzmitryShylovich

This comment has been minimized.

Show comment
Hide comment
@DzmitryShylovich

DzmitryShylovich Feb 22, 2017

Contributor

@michelherv yeah, reproduce on plunkr and file a new issue pls

Contributor

DzmitryShylovich commented Feb 22, 2017

@michelherv yeah, reproduce on plunkr and file a new issue pls

@michelherv

This comment has been minimized.

Show comment
Hide comment
@michelherv

michelherv Feb 22, 2017

@DzmitryShylovich done! You can found the description here #14645

michelherv commented Feb 22, 2017

@DzmitryShylovich done! You can found the description here #14645

@szh

This comment has been minimized.

Show comment
Hide comment
@szh

szh Mar 30, 2017

This is still broken in Angular 4.0. Any plans to fix it?

szh commented Mar 30, 2017

This is still broken in Angular 4.0. Any plans to fix it?

@DzmitryShylovich

This comment has been minimized.

Show comment
Hide comment
@DzmitryShylovich

DzmitryShylovich Mar 30, 2017

Contributor

@szh please don't spam everywhere. Pr is here #13922

Contributor

DzmitryShylovich commented Mar 30, 2017

@szh please don't spam everywhere. Pr is here #13922

DzmitryShylovich added a commit to DzmitryShylovich/angular that referenced this issue Mar 30, 2017

@CRodriguez25

This comment has been minimized.

Show comment
Hide comment
@CRodriguez25

CRodriguez25 May 16, 2017

Is there a work around for this by any chance?

CRodriguez25 commented May 16, 2017

Is there a work around for this by any chance?

@arutnik

This comment has been minimized.

Show comment
Hide comment
@arutnik

arutnik May 24, 2017

I couldn't find any workaround inside my candeactivate class. Still waiting for the PR

arutnik commented May 24, 2017

I couldn't find any workaround inside my candeactivate class. Still waiting for the PR

@skaiser

This comment has been minimized.

Show comment
Hide comment
@skaiser

skaiser May 26, 2017

It seems it should work like resolve does, no? That is, it doesn't navigate until canDeactivate cancels or confirms whether the component can be deactivated (or routed away from). Otherwise, I don't really understand what it's purpose is.

skaiser commented May 26, 2017

It seems it should work like resolve does, no? That is, it doesn't navigate until canDeactivate cancels or confirms whether the component can be deactivated (or routed away from). Otherwise, I don't really understand what it's purpose is.

@CRodriguez25

This comment has been minimized.

Show comment
Hide comment
@CRodriguez25

CRodriguez25 May 26, 2017

Skaiser, its purpose is what you described, but there is a bug currently. A PR has been made to solve the issue, but until then the CanDeactivate guard breaks your history.

CRodriguez25 commented May 26, 2017

Skaiser, its purpose is what you described, but there is a bug currently. A PR has been made to solve the issue, but until then the CanDeactivate guard breaks your history.

@alexeikostevich

This comment has been minimized.

Show comment
Hide comment
@alexeikostevich

alexeikostevich Aug 14, 2017

Our team develops an application that uses browser history heavily to navigate through pages. Unfortunately, we have faced the same issue which causes a bit weird navigation experience for users.
The fix for this issue would be much appreciated!

alexeikostevich commented Aug 14, 2017

Our team develops an application that uses browser history heavily to navigate through pages. Unfortunately, we have faced the same issue which causes a bit weird navigation experience for users.
The fix for this issue would be much appreciated!

@deepender87

This comment has been minimized.

Show comment
Hide comment
@deepender87

deepender87 Sep 27, 2017

Hey Guys
I am facing the same issue, can someone update, when it can be fixed. I am using angular 4.0 and router 4.0

deepender87 commented Sep 27, 2017

Hey Guys
I am facing the same issue, can someone update, when it can be fixed. I am using angular 4.0 and router 4.0

@malthusyau

This comment has been minimized.

Show comment
Hide comment
@malthusyau

malthusyau Oct 16, 2017

Hi all, what's the status on this issue? Or is there any workaround?

malthusyau commented Oct 16, 2017

Hi all, what's the status on this issue? Or is there any workaround?

@awetstone56

This comment has been minimized.

Show comment
Hide comment
@awetstone56

awetstone56 Oct 23, 2017

I am also using Angular 4.0 and Router 4.0. Having this issue as well. This is a major bug and is a big setback for our application. Would be great if a workaround could be found or fix be made to this.

awetstone56 commented Oct 23, 2017

I am also using Angular 4.0 and Router 4.0. Having this issue as well. This is a major bug and is a big setback for our application. Would be great if a workaround could be found or fix be made to this.

@caffeineinc

This comment has been minimized.

Show comment
Hide comment
@caffeineinc

caffeineinc Oct 30, 2017

Still a problem, any ideas ?

caffeineinc commented Oct 30, 2017

Still a problem, any ideas ?

@Kiwi15

This comment has been minimized.

Show comment
Hide comment
@Kiwi15

Kiwi15 Nov 16, 2017

Still have no fix ?

Kiwi15 commented Nov 16, 2017

Still have no fix ?

@patrickracicot

This comment has been minimized.

Show comment
Hide comment
@patrickracicot

patrickracicot Jan 9, 2018

Can someone give the state of this issue?

patrickracicot commented Jan 9, 2018

Can someone give the state of this issue?

@ngbot ngbot bot added this to the Backlog milestone Jan 23, 2018

@IRCraziestTaxi

This comment has been minimized.

Show comment
Hide comment
@IRCraziestTaxi

IRCraziestTaxi Jan 24, 2018

I am also still waiting for an update on this.

IRCraziestTaxi commented Jan 24, 2018

I am also still waiting for an update on this.

@znagatomo

This comment has been minimized.

Show comment
Hide comment
@znagatomo

znagatomo Mar 1, 2018

As a work around while this hasn't been fixed we do a Location.forward() every time DeactivateGuard returns false.

znagatomo commented Mar 1, 2018

As a work around while this hasn't been fixed we do a Location.forward() every time DeactivateGuard returns false.

@alfredorevilla

This comment has been minimized.

Show comment
Hide comment
@alfredorevilla

alfredorevilla Mar 5, 2018

@patrickracicot @DzmitryShylovich i'm using angular 5.x and history gets lost after some by guard cancels. seems issue has not been fixed. has it?

alfredorevilla commented Mar 5, 2018

@patrickracicot @DzmitryShylovich i'm using angular 5.x and history gets lost after some by guard cancels. seems issue has not been fixed. has it?

@Foszcz

This comment has been minimized.

Show comment
Hide comment
@Foszcz

Foszcz Mar 7, 2018

The problem occurs again on version 5.2.4 . Any plans to fix it?

Foszcz commented Mar 7, 2018

The problem occurs again on version 5.2.4 . Any plans to fix it?

@alfredorevilla

This comment has been minimized.

Show comment
Hide comment
@alfredorevilla

alfredorevilla Mar 7, 2018

hey @Foszcz. did it not occur on previous 5.x versions?

alfredorevilla commented Mar 7, 2018

hey @Foszcz. did it not occur on previous 5.x versions?

@jotatoledo

This comment has been minimized.

Show comment
Hide comment
@jotatoledo

jotatoledo Mar 7, 2018

The original reproduction is not working anymore, and its outdated. Ppl having this issue should open a new issue referencing this one.

jotatoledo commented Mar 7, 2018

The original reproduction is not working anymore, and its outdated. Ppl having this issue should open a new issue referencing this one.

@patrickracicot

This comment has been minimized.

Show comment
Hide comment
@patrickracicot

patrickracicot Mar 7, 2018

@jotatoledo What do you mean. The plunkr is still working and the step are still reproductible. If you update the config.js in the plunkr to use the latest version of Angular, you'll see that it still is reproductible.

patrickracicot commented Mar 7, 2018

@jotatoledo What do you mean. The plunkr is still working and the step are still reproductible. If you update the config.js in the plunkr to use the latest version of Angular, you'll see that it still is reproductible.

@evertonverbo

This comment has been minimized.

Show comment
Hide comment
@evertonverbo

evertonverbo Apr 4, 2018

I'm facing the same behaviour here, someone know's when it will be fixed? :(

evertonverbo commented Apr 4, 2018

I'm facing the same behaviour here, someone know's when it will be fixed? :(

@riiight

This comment has been minimized.

Show comment
Hide comment
@riiight

riiight Apr 11, 2018

Also chiming in that this is an issue that is affecting our app.

riiight commented Apr 11, 2018

Also chiming in that this is an issue that is affecting our app.

@Valentin-Paramonov

This comment has been minimized.

Show comment
Hide comment
@Valentin-Paramonov

Valentin-Paramonov Apr 20, 2018

I came up with the following workaround. I doesn't fix forward button navigation (actually, it kinda makes it worse) and I'm sure there are many other problems with it. I haven't done a lot of testing either, so I highly discourage using it, only if you're really desperate.

export class AppModule {
    constructor(location: Location, router: Router) {
        location.subscribe(locationEvent => {
            router.events
                .pipe(
                    filter(event => event instanceof NavigationCancel || event instanceof NavigationEnd),
                    first(),
                    filter(event => event instanceof NavigationCancel),
                    filter((event: NavigationCancel) => event.url === locationEvent.url)
                )
                .subscribe(cancelEvent => {
                    location.replaceState(cancelEvent.url);
                    location.forward();
                });
        });
    }
}

Valentin-Paramonov commented Apr 20, 2018

I came up with the following workaround. I doesn't fix forward button navigation (actually, it kinda makes it worse) and I'm sure there are many other problems with it. I haven't done a lot of testing either, so I highly discourage using it, only if you're really desperate.

export class AppModule {
    constructor(location: Location, router: Router) {
        location.subscribe(locationEvent => {
            router.events
                .pipe(
                    filter(event => event instanceof NavigationCancel || event instanceof NavigationEnd),
                    first(),
                    filter(event => event instanceof NavigationCancel),
                    filter((event: NavigationCancel) => event.url === locationEvent.url)
                )
                .subscribe(cancelEvent => {
                    location.replaceState(cancelEvent.url);
                    location.forward();
                });
        });
    }
}
@gerich-home

This comment has been minimized.

Show comment
Hide comment
@gerich-home

gerich-home Apr 25, 2018

Hi, I was able to prepare a more intrusive fix for the issue by adding manual changes to Angular code of router.umd.js bundle file that is used in our project:
https://gist.github.com/gerich-home/3065d97839c729b49fcbf9c0ff39d9af

The base patched file from Angular 4.4.6 is in the gist:
https://gist.github.com/gerich-home/5bf6d1431bbf887b4e472d4767111944/dbbcf394963ca883d80793c3bb748e596e9216b1

The change fixes few related issues (history is broken, back browser button is not handled correctly after cancelled programmatic navigation, url gets replaced with long 'logical' url after navigation is cancelled)

gerich-home commented Apr 25, 2018

Hi, I was able to prepare a more intrusive fix for the issue by adding manual changes to Angular code of router.umd.js bundle file that is used in our project:
https://gist.github.com/gerich-home/3065d97839c729b49fcbf9c0ff39d9af

The base patched file from Angular 4.4.6 is in the gist:
https://gist.github.com/gerich-home/5bf6d1431bbf887b4e472d4767111944/dbbcf394963ca883d80793c3bb748e596e9216b1

The change fixes few related issues (history is broken, back browser button is not handled correctly after cancelled programmatic navigation, url gets replaced with long 'logical' url after navigation is cancelled)

@555ea

This comment has been minimized.

Show comment
Hide comment
@555ea

555ea May 4, 2018

@gerich-home Could you provide a little guide, how we could use your fixed router in our projects ? I was trying to override Router provider with no success

555ea commented May 4, 2018

@gerich-home Could you provide a little guide, how we could use your fixed router in our projects ? I was trying to override Router provider with no success

@gerich-home

This comment has been minimized.

Show comment
Hide comment
@gerich-home

gerich-home May 7, 2018

Our project is based on https://github.com/vyakymenko/angular-seed-express
There I had to change seed.config.ts file to override systemjs config with pathced file.
Specifically I changed:

'@angular/router': 'node_modules/@angular/router/bundles/router.umd.js',

to

'@angular/router': 'patched-router.umd.js',

Added the first line:

      '@angular/router': join('.', this.APP_DEST, 'router.umd.js'),
      'tslib': 'node_modules/tslib/tslib.js',
      'dist/tmp/node_modules/*': 'dist/tmp/node_modules/*',
      'node_modules/*': 'node_modules/*',
      '*': 'node_modules/*'

And changed

      '@angular/router': {
        main: 'bundles/router.umd.js',
        defaultExtension: 'js'
      },

to

      '@angular/router': {
        main: 'router.umd.js',
        defaultExtension: 'js'
      },

Hope it helps.

gerich-home commented May 7, 2018

Our project is based on https://github.com/vyakymenko/angular-seed-express
There I had to change seed.config.ts file to override systemjs config with pathced file.
Specifically I changed:

'@angular/router': 'node_modules/@angular/router/bundles/router.umd.js',

to

'@angular/router': 'patched-router.umd.js',

Added the first line:

      '@angular/router': join('.', this.APP_DEST, 'router.umd.js'),
      'tslib': 'node_modules/tslib/tslib.js',
      'dist/tmp/node_modules/*': 'dist/tmp/node_modules/*',
      'node_modules/*': 'node_modules/*',
      '*': 'node_modules/*'

And changed

      '@angular/router': {
        main: 'bundles/router.umd.js',
        defaultExtension: 'js'
      },

to

      '@angular/router': {
        main: 'router.umd.js',
        defaultExtension: 'js'
      },

Hope it helps.

@ivucicev

This comment has been minimized.

Show comment
Hide comment
@ivucicev

ivucicev May 22, 2018

Facing the same issue... v5.0.3... tried some workarounds with location.forward, but I always end up breaking some other part of navigation like reload or, browser navigation buttons...

ivucicev commented May 22, 2018

Facing the same issue... v5.0.3... tried some workarounds with location.forward, but I always end up breaking some other part of navigation like reload or, browser navigation buttons...

@ghost

This comment has been minimized.

Show comment
Hide comment
@ghost

ghost May 23, 2018

I am surprised this is still an issue. :) Any updates guys about this issue?

ghost commented May 23, 2018

I am surprised this is still an issue. :) Any updates guys about this issue?

@davemvp

This comment has been minimized.

Show comment
Hide comment
@davemvp

davemvp Jun 14, 2018

Still broken in v6.03.

It seems like the priority for fixing location.back() and location.forward() (#15664) should be the same. Just guessing the same fix could be applied to both methods. 18 months seems like a long time for this issue to be out there.

davemvp commented Jun 14, 2018

Still broken in v6.03.

It seems like the priority for fixing location.back() and location.forward() (#15664) should be the same. Just guessing the same fix could be applied to both methods. 18 months seems like a long time for this issue to be out there.

@icesmith

This comment has been minimized.

Show comment
Hide comment
@icesmith

icesmith Jul 3, 2018

As a workaround, I tried to put the active url back to the history, and this approach seems to work:

export class CanDeactivateGuard implements CanDeactivate<any> {
    constructor(
        private readonly location: Location,
        private readonly router: Router
    ) {}

    canDeactivate(component: any, currentRoute: ActivatedRouteSnapshot): boolean {
        if (myCondition) {
            const currentUrlTree = this.router.createUrlTree([], currentRoute);
            const currentUrl = currentUrlTree.toString();
            this.location.go(currentUrl);
            return false;
        } else {
            return true;
        }
    }
}

icesmith commented Jul 3, 2018

As a workaround, I tried to put the active url back to the history, and this approach seems to work:

export class CanDeactivateGuard implements CanDeactivate<any> {
    constructor(
        private readonly location: Location,
        private readonly router: Router
    ) {}

    canDeactivate(component: any, currentRoute: ActivatedRouteSnapshot): boolean {
        if (myCondition) {
            const currentUrlTree = this.router.createUrlTree([], currentRoute);
            const currentUrl = currentUrlTree.toString();
            this.location.go(currentUrl);
            return false;
        } else {
            return true;
        }
    }
}
@Rzpeg

This comment has been minimized.

Show comment
Hide comment
@Rzpeg

Rzpeg Jul 9, 2018

@icesmith this works, thanks!

Rzpeg commented Jul 9, 2018

@icesmith this works, thanks!

@chrisblay

This comment has been minimized.

Show comment
Hide comment
@chrisblay

chrisblay Aug 15, 2018

Any update on this issue?

The behavior of the Angular Guide's primary router example (Crisis Center) is broken. It does not match the user-reported behavior described above, and also does not appear to match intended expected behavior. In the Crisis Center example/stackblitz:

  1. Clicking the simulated browser Back/Forward buttons does not appear to trigger the CanDeactivate guard.
  2. Clicking a button within the app (e.g. going from "crisis-center/1" to "crisis-center/2") does trigger the CanDeactivate guard, then selecting Cancel (e.g. discard changes) works as expected, i.e. does not impact the URL history.

See:
https://angular.io/guide/router#routing--navigation
https://angular.io/generated/live-examples/router/stackblitz.html


In my app, I don't believe CanDeactivate guards can be used at all while this behavior is broken.

If there is an accurate workaround, can the Angular Guide (and its example) be updated to include it? Or at least, add a Known Issues section describing why users following the tutorial will not see expected behavior?

chrisblay commented Aug 15, 2018

Any update on this issue?

The behavior of the Angular Guide's primary router example (Crisis Center) is broken. It does not match the user-reported behavior described above, and also does not appear to match intended expected behavior. In the Crisis Center example/stackblitz:

  1. Clicking the simulated browser Back/Forward buttons does not appear to trigger the CanDeactivate guard.
  2. Clicking a button within the app (e.g. going from "crisis-center/1" to "crisis-center/2") does trigger the CanDeactivate guard, then selecting Cancel (e.g. discard changes) works as expected, i.e. does not impact the URL history.

See:
https://angular.io/guide/router#routing--navigation
https://angular.io/generated/live-examples/router/stackblitz.html


In my app, I don't believe CanDeactivate guards can be used at all while this behavior is broken.

If there is an accurate workaround, can the Angular Guide (and its example) be updated to include it? Or at least, add a Known Issues section describing why users following the tutorial will not see expected behavior?

@raczosala

This comment has been minimized.

Show comment
Hide comment
@raczosala

raczosala Aug 15, 2018

@icesmith solution won't work with named router outlets

raczosala commented Aug 15, 2018

@icesmith solution won't work with named router outlets

@DoanVanThuong

This comment has been minimized.

Show comment
Hide comment
@DoanVanThuong

DoanVanThuong Oct 11, 2018

geeting same error wit Angular App v4

DoanVanThuong commented Oct 11, 2018

geeting same error wit Angular App v4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment