Deactivation Guard breaks the routing history

patrickracicot commented 7 years ago

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

Click button Nav to route1
Click button Nav to route2
Click button Nav to route3
Click button Block Nav Back
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 commented 7 years ago

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 commented 7 years ago

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.

michelherv commented 7 years ago

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 commented 7 years ago

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

michelherv commented 7 years ago

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

szh commented 7 years ago

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

DzmitryShylovich commented 7 years ago

@szh please don't spam everywhere. Pr is here https://github.com/angular/angular/pull/13922

CRodriguez25 commented 7 years ago

Is there a work around for this by any chance?

arutnik commented 7 years ago

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

skaiser commented 7 years ago

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 commented 7 years ago

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.

alexeykostevich commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

awetstone56 commented 6 years ago

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 commented 6 years ago

Still a problem, any ideas ?

loschmitz commented 6 years ago

Still have no fix ?

patrickracicot commented 6 years ago

Can someone give the state of this issue?

IRCraziestTaxi commented 6 years ago

I am also still waiting for an update on this.

znagatomo commented 6 years ago

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

alfredorevilla commented 6 years ago

@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 commented 6 years ago

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

alfredorevilla commented 6 years ago

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

jotatoledo commented 6 years ago

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

patrickracicot commented 6 years ago

@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 commented 6 years ago

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

riiight commented 6 years ago

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

ValentinParamonov commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

davemvp commented 6 years ago

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 commented 5 years ago

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 commented 5 years ago

@icesmith this works, thanks!

chrisblay commented 5 years ago

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:

Clicking the simulated browser Back/Forward buttons does not appear to trigger the CanDeactivate guard.
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 commented 5 years ago

@icesmith solution won't work with named router outlets

DoanVanThuong commented 5 years ago

geeting same error wit Angular App v4

benceHornyak commented 5 years ago

This is my workaround with a custom dialog component, based on @icesmith solution:

// can-deactivate-guard.ts
import { Injectable } from '@angular/core';
import { CanDeactivate, ActivatedRouteSnapshot, Router } from '@angular/router';
import { DeactivationGuarded } from '../deactivation-guarded';
import { Observable, of } from 'rxjs';
import { Location } from '@angular/common';
import { switchMap } from 'rxjs/operators';

@Injectable({
    providedIn: 'root'
})
export class CanDeactivateGuard implements CanDeactivate<DeactivationGuarded> {

    constructor(
        private readonly router: Router,
        private readonly location: Location,
    ) {}

    canDeactivate(component: DeactivationGuarded, currentRoute: ActivatedRouteSnapshot): Observable<boolean> {

        return component.canDeactivate().pipe(
            switchMap((resultFromConfirmDialog: boolean) => {
            if (!resultFromConfirmDialog) {
                const currentUrlTree = this.router.createUrlTree([], currentRoute);
                const currentUrl = currentUrlTree.toString();
                this.location.go(currentUrl);
                return of(false);
            }
            return of(true);
        }));
    }
}

Guarded interface:

import { Observable } from 'rxjs';

export interface DeactivationGuarded {
    canDeactivate(): Observable<boolean>;
}

And I have this in my component which I want to protect(component has to implement DeactivationGuarded):

canDeactivate(): Observable<boolean> {
    console.log('canDeactivate has fired in the component!');
    const dialogRef = this.dialog.open(ConfirmDialogComponent);
    return dialogRef.afterClosed();
}