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

Lazy Loading Feature Module with Guards causing Infinite Navigation Loop #1781

Closed
BuairtRi opened this issue Apr 22, 2019 · 9 comments
Closed

Comments

@BuairtRi
Copy link

BuairtRi commented Apr 22, 2019

I have a problem where, if the initial request I make to my application is to a route that requires you to be authenticated, and you're not authenticated - the guard shoots you to the login page, which is inside the Authentication feature module - fairly run-of-the-mill logic.

The problem is, the router store is running code which - before the feature module routes are loaded/recognized - attempts to navigate me back to the route. Basically, at some point the Route State is being updated, but the route url is an empty string because the routes haven't been recognized yet.

The routes are set up like this:

export const appRoutes : Routes = [
    {
        path         : 'authentication',
        component    : ExternalLayoutComponent,
        loadChildren : './features/authentication/authentication.module#AuthenticationModule'
    },
    {
        path         : 'users',
        canActivate  : [ RoleGuard ],
        loadChildren : './features/users/users.module#UsersModule',
        data         : {
            roles : [ 'Administrator' ]
        }
    },
    {
        path       : '**',
        redirectTo : 'authentication'
    }
];
export const userRoutes : Routes = [
    {
        path       : '',
        redirectTo : 'list'
    },
    {
        path      : 'list',
        component : UsersListComponent
    },
    {
        path      : 'profile/:id',
        component : ProfileComponent
    }
];
export const authenticationRoutes : Routes = [
    {
        path      : '',
        component : LoginComponent,
        pathMatch : 'full'
    },
    {
        path      : 'login',
        component : LoginComponent
    }
];

So if I request '/users/list', everything works okay until the guard determines the route can't be activated.

The guard redirects the user to the login page via a call like: router.navigate(['/authentication/login'])

The router goes out, loads the module and gets to the "RouteConfigLoadEnded" router event. At this point, the Store calls update-reducers to load the store state for that feature module. However, the URL in the route is an empty string at this point, rather than '/authentication/login`. The update-reducers action causes the router-state reducer to execute (I think), which causes the store to update the routerstate URL value to "".

The state update triggers this code in the StoreRouterConnectingModule to execute:

private navigateIfNeeded(
    routerStoreState: RouterReducerState,
    storeState: any
  ): void {
    if (!routerStoreState || !routerStoreState.state) {
      return;
    }
    if (this.trigger === RouterTrigger.ROUTER) {
      return;
    }
    if (this.lastEvent instanceof NavigationStart) {
      return;
    }

    const url = routerStoreState.state.url;
    if (this.router.url !== url) {
      this.storeState = storeState;
      this.trigger = RouterTrigger.STORE;
      this.router.navigateByUrl(url).catch(error => {
        this.errorHandler.handleError(error);
      });
    }
  }

The this.router.navigateByUrl(url) section at the end is getting triggered, which is NOT what I would expect. This causes an infinite loop, because an empty string redirects right back to '/authentication/login' route, which causes the whole sequence to fire again - and again, etc.

Expected behavior:

The router-state should not update the URL until after the RoutesRecognized event, because the url in the router isn't accurate until the RoutesRecognized event.

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

Angular: 7.2.13
@ngrx/store : 7.4
@ngrx/router-store: 7.4

Other information:

I was able to fix this problem if I just add another if statement in the navigateIfNeeded function:

private navigateIfNeeded(
    routerStoreState: RouterReducerState,
    storeState: any
  ): void {
    if (!routerStoreState || !routerStoreState.state) {
      return;
    }
    if (this.trigger === RouterTrigger.ROUTER) {
      return;
    }
    if (this.lastEvent instanceof NavigationStart) {
      return;
    }
    if (this.lastEvent instanceof RouteConfigLoadEnd) {
            return;
    }

    const url = routerStoreState.state.url;
    if (this.router.url !== url) {
      this.storeState = storeState;
      this.trigger = RouterTrigger.STORE;
      this.router.navigateByUrl(url).catch(error => {
        this.errorHandler.handleError(error);
      });
    }
  }

But I have absolutely no idea if this solution will cause weird side effects or not. I'm fairly new to ngrx and the ngrx RouterStoreConnectorModule, so I can't tell if this:

a) an oversight on my part in the structure of the routes (although I've combed through pretty intensely, and I don't think it is)

b) The ngrx/router-store module is assuming that guards will redirect to a non-feature module route (or
a previously loaded feature module). In that case the router shouldn't have to load the RouteConfig data for the guard redirected route, and therefore the redirection goes through smoothly. I would consider this a bug, because I can imagine lots of scenarios where guards could redirect you to parts of an app you haven't visited before.

c) B, but my solution is super hacky and would cause weird side effects.

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

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

@timdeschryver
Copy link
Member

Could you provide a reproduction for this?
I would have expected the same outcome in the example app https://github.com/ngrx/platform/blob/master/projects/example-app/src/app/auth/services/auth-guard.service.ts

@BuairtRi
Copy link
Author

@timdeschryver - I'll work on writing up a reproduction for you.

But looking at the example app, I am fairly sure the reason you don't see it there is that the AuthModule is imported into the AppModule directly. It's not lazy loaded as a feature via the router's lazy loading syntax. That means that the routes for the login page are already loaded and configured BEFORE the guard triggers the redirection.

@PhelanHarris
Copy link

I have this same issue. The problem seems to definitely be in the navigateIfNeeded function during that moment where angular is loading the lazy loaded module and the router state's url is an empty string.
This statement:

const url = routerStoreState.state.url;
if (this.router.url !== url) {

evaluates true because url is "" and this.router.url is "/".

I was able to come up with a workaround in my code by implementing a custom route state serializer and storing a slash instead of an empty string when the route snapshot returns an empty string:

export class CustomRouterStateSerializer implements RouterStateSerializer<CustomRouterState> {
  serialize(routerState: RouterStateSnapshot): CustomRouterState {
    let route = routerState.root

    while (route.firstChild) {
      route = route.firstChild
    }

    const state: CustomRouterState = {
      url: routerState.url || '/',                  // this is the important line
      params: route.params,
      queryParams: routerState.root.queryParams,
      data: route.data
    }
    return state
  }
}

And then importing it in my app module like so:

StoreRouterConnectingModule.forRoot({
  serializer: CustomRouterStateSerializer
})

I'm not sure if this has any side effects or if it'll work for everyone but might be worth a try for anyone else with this issue

@MarkoSulamagi
Copy link

I have the same issue. @PhelanHarris workaround fixed it for me as well.
Thanks!

@timdeschryver
Copy link
Member

@BuairtRi could you try out @PhelanHarris 's fix?
Maybe we should default to / instead of an empty string because Angular's first route navigation is to / if I'm not mistaken.

@jbale
Copy link

jbale commented Mar 17, 2020

I ran into this problem today and thought i would throw my 2 pence worth in in case it can help.

I found that the router state is serialised using "this.router.routerState.snapshot"

if (event instanceof NavigationStart) {
this.routerState = this.serializer.serialize(
this.router.routerState.snapshot
);
if (this.trigger !== RouterTrigger.STORE) {
this.storeState = storeState;
this.dispatchRouterRequest(event);
}
} else if (event instanceof RoutesRecognized) {

Then when navigateIfNeeded is called on state change this.router.url is used to check if navigation is needed.

const url = routerStoreState.state.url;
if (this.router.url !== url) {
this.storeState = storeState;
this.trigger = RouterTrigger.STORE;
this.router.navigateByUrl(url).catch(error => {
this.errorHandler.handleError(error);
});
}

For some reason accessing the url in these different way yields different values when the current route is empty route.

this.router.routerState.snapshot.url returns ""
this.router.url returns "/"

This started happening as a result of adding a new meta reducer. When the store initialises or loads a new feature module it would rehydrate the state from local storage. I think this was causing the state change event so it would attempt to check if navigation is needed and would successfully try to navigate as '' !== '/'

I have implemented the work around for now but seems like we should be consistent in how we access the url.

@brandonroberts
Copy link
Member

Please test against the latest release and reopen if this is still an issue

@jorgeizquierdo
Copy link

jorgeizquierdo commented Aug 2, 2020

I just, test with 9.2.0 release and I have the same issue. I solved it with @PhelanHarris' workaround.

@chan-dev
Copy link

i had the same issue and @PhelanHarris workaround worked. Thanks

chan-dev added a commit to chan-dev/pocketlite that referenced this issue Oct 10, 2020
Basically, everytime the route is an empty string replace it with "/"

Related issue:
ngrx/platform#1781 (comment)

Fixes issue: #8
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

8 participants