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

After Angular 7 upgrade I started having problems with the Store Router Connector #1373

Closed
jbeckton opened this issue Oct 19, 2018 · 16 comments

Comments

@jbeckton
Copy link

Minimal reproduction of the bug/regression with instructions:

Stackblitz seed is still on Angular 6.1.0, this occurs with Angular 7.0

When using a Route Guard to check if user is authenticated I will get the following error if the user is not authenticated and the Guard logic return false and I redirect to a login route.

The browser console reports:
Throttling history state changes to prevent the browser from hanging.

export const ROUTES: Routes = [
  {
    path: '',
    component: FeaturesContainerComponent,
    children: [
      { path: '', redirectTo: 'dashboard', pathMatch: 'full' },
      {
        path: 'dashboard',
        canActivate: [AuthenticationGuard],
        component: DashboardComponent
      },
    ]
  },
  {
    path: 'login', // not secured
    component: LoginComponent
  }, {
    path: 'logout', // not secured
    component: LogoutComponent
  }
];

authentication.guard.ts

canActivate(route: ActivatedRouteSnapshot, state: RouterStateSnapshot): Observable<boolean> {
    return this.checkIsAuthenticated();
}

public checkIsAuthenticated(): Observable<boolean> {

    return new Observable((observer) => {

        this.store.pipe(
            select(authenticationStateQuery.selectCheckIsAuthenticatedResult),
            takeUntil(this.unsubscribeSubject)
        )
        .subscribe((state: CheckIsAuthenticatedStatus) => {

                if (state) {
                    if (state.result) {
                       observer.next(true);
                       observer.complete();
                    } else {
                        // This part causes infinite loop and crashes browser.
                        observer.next(false);
                        observer.complete();
                        this.router.navigate(['/login']);
                    }
                    this.unsubscribeSubject.next(false);
                }
            });
        this.store.dispatch(new CheckIsAuthenticatedAction());
        
    }); 
}

Expected behavior:

The guard should work as it did with Angular 6, if the Guard logic determines the user is not authenticated then returning false to the router and then calling navigate should take me to the desired redirect without errors.

After disconnecting the StoreRouterConnectingModule from my app it works as expected.

Versions of NgRx, Angular, Node, affected browser(s) and operating system(s):

NGRX: v6.1.0
Angular: v7
Node: v8.9.0
Chrome
Linux CentOs 7.5

Other information:

I would be willing to submit a PR to fix this issue

[ ] Yes (Assistance is provided if you need help submitting a pull request)
[ x] No

@timdeschryver
Copy link
Member

This should be resolved when we finished the upgrade to Angular 7.

@aalmazanarbs
Copy link

This should be resolved when we finished the upgrade to Angular 7.

Estimated time?

@LeoCreer
Copy link

Any Estimated time when 7 will be out?

@xigolle
Copy link

xigolle commented Oct 24, 2018

Got the same issue.
Somebody has a workaround we are trying to update to Angular 7 (Coming from version 5) but it will not work.

Or is there any way I can update to version Angular 6.1?

@wesselvdv
Copy link

Any workaround for now?

@brandonroberts
Copy link
Member

Fixed via #1379

6.1.1 will be published today with the fix

@brandonroberts
Copy link
Member

6.1.1 has been published on npm

@wesselvdv
Copy link

Weird, I am actually still getting an infinite loop with 6.1.1, and a similar error in Safari as the op.

@timdeschryver
Copy link
Member

Could you reproduce it in a repo or in stackblitz?

@pedep
Copy link

pedep commented Oct 31, 2018

@timdeschryver Try this https://github.com/pedep/ngrx-router-crash

I can get this to crash my tab pretty consistently on my 2012mbp/firefox63 and my thinkpadx220+linux/chrome55
You have to navigate from another page (like about:blank) to /admin, which should lead to an infinite loop

ROUTER_CANCEL triggers this

which triggers this (chain hops off somewhere, leading to this.dispatchTriggeredByRouter being )false, never being set true again.

if (this.dispatchTriggeredByRouter) return;
if (this.router.url !== this.storeState[this.stateKey].state.url) {
this.navigationTriggeredByDispatch = true;
this.router.navigateByUrl(this.storeState[this.stateKey].state.url);

The navigation redirects to the guarded url again, triggering another ROUTER_CANCEL, beginning over at the top.

For some reason, this.routerState does not update with the route config for /user/sign_in before the loop begins.

this.routerState = this.serializer.serialize(routerState);

If it succeeds without hanging the browser, you can try refresh or refresh with devtools closed (which is a factor for some reason...), or a incognito window.

In my own attempt at understanding the issue, i tried adding console.log statements here,
at the this.dispatchTriggeredByRouter = true and this.dispatchTriggeredByRouter = false line

this.dispatchTriggeredByRouter = true;
try {
this.store.dispatch({ type, payload });
} finally {
this.dispatchTriggeredByRouter = false;
this.navigationTriggeredByDispatch = false;

and also here

in the success case ( no loop ) it goes
1 this.dispatchTriggeredByRouter = true has happened
2 inside reducer
3 this.dispatchTriggeredByRouter = false has happened

while the failure case (infinite loop -> crash tab) it goes
1 this.dispatchTriggeredByRouter = true has happened
2 this.dispatchTriggeredByRouter = false has happened
3 inside reducer

@timdeschryver timdeschryver reopened this Oct 31, 2018
@timdeschryver
Copy link
Member

Thanks @pedep !

@timdeschryver
Copy link
Member

This is my bad.

While testing the fix I was still using Angular 6, resulting in a false positive.

@brandonroberts when #1392 gets merged, we'll need another release.

@brandonroberts
Copy link
Member

Fixed via #1392 and published in 6.1.2

@binu101
Copy link

binu101 commented Nov 29, 2018

I am using the 7.0.0-beta and also encounter this issue. Is this also fixes in the 7.0 release?

@nguyenbathanh
Copy link

The fix on version 6.1.2 doesn't work, still get the same issue.

Angular 7.0.1.

@timdeschryver
Copy link
Member

I couldn't reproduce it, feel free to open up a new issue with a reproduction.

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

No branches or pull requests

10 participants