ngx-rocket / generator-ngx-rocket

:rocket: Extensible Angular 14+ enterprise-grade project generator
https://ngx-rocket.github.io/
MIT License
1.53k stars 216 forks source link

Navigate to home after login does not work with Angular 11 #576

Closed csname1910 closed 3 years ago

csname1910 commented 4 years ago

I'm submitting a...

Current behavior

After upgrade to Angular 11 the login button does not work anymore. URL in Chrome is updated to 'home', but it stays on the login screen. You have to hit F5 for refresh to get to the home screen.

Expected behavior

After pressing login button the home screen should appear.

Minimal reproduction of the problem with instructions

ngx new ng update @angular/cli @angular/core npm start

The problem seems to be a change in shouldReuseRoute, see here:

https://update.angular.io/?l=3&v=10.0-11.0

The bug can be fixed by changing shouldReuseRoute to:

public shouldReuseRoute(future: ActivatedRouteSnapshot, curr: ActivatedRouteSnapshot): boolean {
  return future.routeConfig === curr.routeConfig || (curr.url.length > 0 ? curr.data.reuse : future.data.reuse);
}

But this is probably not a good way to fix it.

Environment



ngX-Rocket: 9.1.0
Node.js: v12.18.0
Npm: 6.14.8
OS: win32 x64 10.0.19042

Generated project options:
{
  "generator-ngx-rocket": {
    "version": "9.1.0",
    "props": {
      "location": "path",
      "strict": false,
      "skipInstall": false,
      "skipQuickstart": false,
      "initGit": true,
      "usePrefix": true,
      "appName": "testnol",
      "target": [
        "web"
      ],
      "pwa": false,
      "ui": "material",
      "layout": "side-menu",
      "auth": true,
      "lazy": false,
      "angulartics": false,
      "languages": [
        "en-US"
      ],
      "tools": [
        "prettier",
        "hads"
      ],
      "utility": [],
      "deploy": "none",
      "projectName": "testnol",
      "packageManager": "npm",
      "mobile": [],
      "desktop": []
    }
  }
}

     _                      _                 ____ _     ___
    / \   _ __   __ _ _   _| | __ _ _ __     / ___| |   |_ _|
   / △ \ | '_ \ / _` | | | | |/ _` | '__|   | |   | |    | |
  / ___ \| | | | (_| | |_| | | (_| | |      | |___| |___ | |
 /_/   \_\_| |_|\__, |\__,_|_|\__,_|_|       \____|_____|___|
                |___/

Angular CLI: 11.0.2
Node: 12.18.0
OS: win32 x64

Angular: 11.0.2
... animations, cli, common, compiler, compiler-cli, core, forms
... language-service, localize, platform-browser
... platform-browser-dynamic, router
Ivy Workspace: Yes

Package                         Version
---------------------------------------------------------
@angular-devkit/architect       0.1001.7
@angular-devkit/build-angular   0.1001.7
@angular-devkit/core            10.1.7
@angular-devkit/schematics      11.0.2
@angular/cdk                    10.2.7
@angular/flex-layout            10.0.0-beta.32
@angular/material               10.2.7
@schematics/angular             11.0.2
@schematics/update              0.1100.2
rxjs                            6.6.3
typescript                      4.0.5

Others:

athisun commented 4 years ago

I've just run into this issue as well. Forgive me if I'm wrong, but the change documented on https://update.angular.io/?l=3&v=10.0-11.0 seems to suggest that we should be able to solve this problem by changing shouldReuseRoute to:

public shouldReuseRoute(future: ActivatedRouteSnapshot, curr: ActivatedRouteSnapshot): boolean {
  return future.routeConfig === curr.routeConfig || curr.data.reuse;
}

Notice that future.data.reuse is now curr.data.reuse.

Quote for reference:

If you use the Router's RouteReuseStrategy, the argument order has changed. When calling RouteReuseStrategy#shouldReuseRoute previously when evaluating child routes, they would be called with the future and current arguments swapped. If your RouteReuseStrategy relies specifically on only the future or current snapshot state, you may need to update the shouldReuseRoute implementation's use of future and current ActivateRouteSnapshots.

csname1910 commented 4 years ago

I've just run into this issue as well. Forgive me if I'm wrong, but the change documented on https://update.angular.io/?l=3&v=10.0-11.0 seems to suggest that we should be able to solve this problem by changing shouldReuseRoute to:

public shouldReuseRoute(future: ActivatedRouteSnapshot, curr: ActivatedRouteSnapshot): boolean {
  return future.routeConfig === curr.routeConfig || curr.data.reuse;
}

This does not work, because the arguments are not always swapped, but only for child routes. You have to change like this:

public shouldReuseRoute(future: ActivatedRouteSnapshot, curr: ActivatedRouteSnapshot): boolean {
  return future.routeConfig === curr.routeConfig || (curr.url.length > 0 ? curr.data.reuse : future.data.reuse);
}

But this looks like a hack, there should be a better way.

athisun commented 4 years ago

@csname1910 Yes, you're right. I've worked with your fix and it seems to break for me in cases where we navigate from a child route with a reusable component (i.e. shell) to a route with non-reused components (e.g. from home to login). For now I've simplified to the following, which fixes navigation but likely breaks component reuse:

return future.routeConfig === curr.routeConfig;
csname1910 commented 4 years ago

@csname1910 Yes, you're right. I've worked with your fix and it seems to break for me in cases where we navigate from a child route with a reusable component (i.e. shell) to a route with non-reused components (e.g. from home to login). For now I've simplified to the following, which fixes navigation but likely breaks component reuse:

return future.routeConfig === curr.routeConfig;

Yes, this breaks component reuse, so the ShellComponent is always recreated. This is bad, because if you use a MatSidenav in ShellComponent it is always closed if you switch to another route.

I could not reproduce your problem with navigating from home to login with my fix. For me it does work.

athisun commented 4 years ago

For reference, this is the original issue: https://github.com/angular/angular/issues/18374

@csname1910 Does atscott's solution work for you? https://stackblitz.com/edit/angular-ivy-ksf9cm

// Reuse the route if the RouteConfig is the same, or if both routes use the
// same component, because the latter can have different RouteConfigs.
return future.routeConfig === curr.routeConfig ||
    (!!future.routeConfig?.component &&
      future.routeConfig?.component === curr.routeConfig?.component);
csname1910 commented 4 years ago
// Reuse the route if the RouteConfig is the same, or if both routes use the
// same component, because the latter can have different RouteConfigs.
return future.routeConfig === curr.routeConfig ||
    (!!future.routeConfig?.component &&
      future.routeConfig?.component === curr.routeConfig?.component);

@athisun This does work for me, thank you!