Skip to content

Commit

Permalink
fix(router): Do not call preload method when not necessary (#47007)
Browse files Browse the repository at this point in the history
In Angular 14, we introduced the `loadComponent` API for a `Route` to
allow lazy loading of a routed component in addition to the existing
`loadChildren` which allows lazy loading of child routes. As a result,
the `preload` method of the `PreloadingStrategy` needs to sometimes be
called even when there is a `canLoad` guard on the `Route`. `CanLoad`
guards block loading of child routes but _do not_ block loading of the
component.

This change updates the conditional checks in the internal preloader to
skip calling the `PreloadingStrategy.preload` when there is only a
`loadChildren` callback with a `canLoad` guard an no `loadComponent`.
In this case, the callback passed to the `preload` method is already
effectively a no-op so it's not necessary to call it at all.

resolves #47003

PR Close #47007
  • Loading branch information
atscott authored and AndrewKushnir committed Aug 2, 2022
1 parent 4e9492c commit 79825d3
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 5 deletions.
4 changes: 2 additions & 2 deletions aio/content/guide/router-tutorial-toh.md
Expand Up @@ -2382,12 +2382,12 @@ Currently, the `AdminModule` does not preload because `CanLoad` is blocking it.

<a id="preload-canload"></a>

#### `CanLoad` blocks preload
#### `CanLoad` blocks preload of children

The `PreloadAllModules` strategy does not load feature areas protected by a [CanLoad](#can-load-guard) guard.

You added a `CanLoad` guard to the route in the `AdminModule` a few steps back to block loading of that module until the user is authorized.
That `CanLoad` guard takes precedence over the preload strategy.
That `CanLoad` guard takes precedence over the preload strategy for loading children routes.

If you want to preload a module as well as guard against unauthorized access, remove the `canLoad()` guard method and rely on the [canActivate()](#can-activate-guard) guard alone.

Expand Down
10 changes: 9 additions & 1 deletion packages/router/src/router_preloader.ts
Expand Up @@ -110,7 +110,15 @@ export class RouterPreloader implements OnDestroy {
const injectorForCurrentRoute = route._injector ?? injector;
const injectorForChildren = route._loadedInjector ?? injectorForCurrentRoute;

if ((route.loadChildren && !route._loadedRoutes) ||
// Note that `canLoad` is only checked as a condition that prevents `loadChildren` and not
// `loadComponent`. `canLoad` guards only block loading of child routes by design. This
// happens as a consequence of needing to descend into children for route matching immediately
// while component loading is deferred until route activation. Because `canLoad` guards can
// have side effects, we cannot execute them here so we instead skip preloading altogether
// when present. Lastly, it remains to be decided whether `canLoad` should behave this way
// at all. Code splitting and lazy loading is separate from client-side authorization checks
// and should not be used as a security measure to prevent loading of code.
if ((route.loadChildren && !route._loadedRoutes && route.canLoad === undefined) ||
(route.loadComponent && !route._loadedComponent)) {
res.push(this.preloadConfig(injectorForCurrentRoute, route));
} else if (route.children || route._loadedRoutes) {
Expand Down
15 changes: 13 additions & 2 deletions packages/router/test/router_preloader.spec.ts
Expand Up @@ -39,7 +39,7 @@ describe('RouterPreloader', () => {
});
});

describe('should not load configurations with canLoad guard', () => {
describe('configurations with canLoad guard', () => {
@NgModule({
declarations: [LazyLoadedCmp],
imports: [RouterModule.forChild([{path: 'LoadedModule1', component: LazyLoadedCmp}])]
Expand All @@ -56,7 +56,7 @@ describe('RouterPreloader', () => {
});


it('should work',
it('should not load children',
fakeAsync(inject([RouterPreloader, Router], (preloader: RouterPreloader, router: Router) => {
preloader.preload().subscribe(() => {});

Expand All @@ -65,6 +65,17 @@ describe('RouterPreloader', () => {
const c = router.config;
expect((c[0] as any)._loadedConfig).not.toBeDefined();
})));

it('should not call the preloading method because children will not be loaded anyways',
fakeAsync(() => {
const preloader = TestBed.inject(RouterPreloader);
const preloadingStrategy = TestBed.inject(PreloadingStrategy);
spyOn(preloadingStrategy, 'preload').and.callThrough();
preloader.preload().subscribe(() => {});

tick();
expect(preloadingStrategy.preload).not.toHaveBeenCalled();
}));
});

describe('should preload configurations', () => {
Expand Down

0 comments on commit 79825d3

Please sign in to comment.