From 95ad452e54a253769669c953a6dfd07421315d41 Mon Sep 17 00:00:00 2001 From: Paul Gammans Date: Wed, 22 Apr 2020 12:32:44 +0100 Subject: [PATCH] fix(router): fix load interaction of navigation and preload strategies (#40389) Fix router to ensure that a route module is only loaded once especially in relation to the use of preload strategies with delayed or partial loading. Add test to check the interaction of PreloadingStrategy and normal router navigation under differing scenarios. Checking: * Prevention of duplicate loading of modules. related to #26557 * Prevention of duplicate RouteConfigLoad(Start|End) events related to #22842 * Ensuring preload strategy remains active for submodules if needed The selected preload strategy should still decide when to load submodules * Possibility of memory leak with unfinished preload subscription related to #26557 * Ensure that the stored loader promise is cleared so that subsequent load will try the fetch again. * Add error handle error from loadChildren * Ensure we handle error from with NgModule create Fixes #26557 #22842 #26557 PR Close #40389 --- goldens/size-tracking/aio-payloads.json | 6 +- .../size-tracking/integration-payloads.json | 4 +- packages/router/src/apply_redirects.ts | 11 +- packages/router/src/config.ts | 5 + packages/router/src/router_config_loader.ts | 50 +-- packages/router/src/router_preloader.ts | 3 +- packages/router/test/apply_redirects.spec.ts | 39 +++ packages/router/test/router_preloader.spec.ts | 311 +++++++++++++++++- 8 files changed, 398 insertions(+), 31 deletions(-) diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json index 0520b53e03..d42dee64fe 100755 --- a/goldens/size-tracking/aio-payloads.json +++ b/goldens/size-tracking/aio-payloads.json @@ -12,7 +12,7 @@ "master": { "uncompressed": { "runtime-es2015": 3033, - "main-es2015": 447514, + "main-es2015": 448036, "polyfills-es2015": 52493 } } @@ -21,9 +21,9 @@ "master": { "uncompressed": { "runtime-es2015": 3153, - "main-es2015": 432078, + "main-es2015": 432647, "polyfills-es2015": 52493 } } } -} +} \ No newline at end of file diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index 3a456cfc71..aba3add9a9 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/goldens/size-tracking/integration-payloads.json @@ -39,7 +39,7 @@ "master": { "uncompressed": { "runtime-es2015": 2285, - "main-es2015": 241202, + "main-es2015": 241843, "polyfills-es2015": 36709, "5-es2015": 745 } @@ -66,4 +66,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/router/src/apply_redirects.ts b/packages/router/src/apply_redirects.ts index 98f195099c..f908d45c66 100644 --- a/packages/router/src/apply_redirects.ts +++ b/packages/router/src/apply_redirects.ts @@ -280,11 +280,12 @@ class ApplyRedirects { segments: UrlSegment[], outlet: string): Observable { if (route.path === '**') { if (route.loadChildren) { - return this.configLoader.load(ngModule.injector, route) - .pipe(map((cfg: LoadedRouterConfig) => { - route._loadedConfig = cfg; - return new UrlSegmentGroup(segments, {}); - })); + const loaded$ = route._loadedConfig ? of(route._loadedConfig) : + this.configLoader.load(ngModule.injector, route); + return loaded$.pipe(map((cfg: LoadedRouterConfig) => { + route._loadedConfig = cfg; + return new UrlSegmentGroup(segments, {}); + })); } return of(new UrlSegmentGroup(segments, {})); diff --git a/packages/router/src/config.ts b/packages/router/src/config.ts index 86af574f02..fbfd9a7f5d 100644 --- a/packages/router/src/config.ts +++ b/packages/router/src/config.ts @@ -483,6 +483,11 @@ export interface Route { * @internal */ _loadedConfig?: LoadedRouterConfig; + /** + * Filled for routes with `loadChildren` during load + * @internal + */ + _loader$?: Observable; } export class LoadedRouterConfig { diff --git a/packages/router/src/router_config_loader.ts b/packages/router/src/router_config_loader.ts index e9d21974ee..0810b785a9 100644 --- a/packages/router/src/router_config_loader.ts +++ b/packages/router/src/router_config_loader.ts @@ -7,8 +7,8 @@ */ import {Compiler, InjectFlags, InjectionToken, Injector, NgModuleFactory, NgModuleFactoryLoader} from '@angular/core'; -import {from, Observable, of} from 'rxjs'; -import {map, mergeMap} from 'rxjs/operators'; +import {ConnectableObservable, from, Observable, of, Subject} from 'rxjs'; +import {catchError, map, mergeMap, refCount, tap} from 'rxjs/operators'; import {LoadChildren, LoadedRouterConfig, Route} from './config'; import {flatten, wrapIntoObservable} from './utils/collection'; @@ -28,27 +28,39 @@ export class RouterConfigLoader { private onLoadEndListener?: (r: Route) => void) {} load(parentInjector: Injector, route: Route): Observable { + if (route._loader$) { + return route._loader$; + } + if (this.onLoadStartListener) { this.onLoadStartListener(route); } - const moduleFactory$ = this.loadModuleFactory(route.loadChildren!); - - return moduleFactory$.pipe(map((factory: NgModuleFactory) => { - if (this.onLoadEndListener) { - this.onLoadEndListener(route); - } - - const module = factory.create(parentInjector); - - // When loading a module that doesn't provide `RouterModule.forChild()` preloader will get - // stuck in an infinite loop. The child module's Injector will look to its parent `Injector` - // when it doesn't find any ROUTES so it will return routes for it's parent module instead. - return new LoadedRouterConfig( - flatten(module.injector.get(ROUTES, undefined, InjectFlags.Self | InjectFlags.Optional)) - .map(standardizeConfig), - module); - })); + const loadRunner = moduleFactory$.pipe( + map((factory: NgModuleFactory) => { + if (this.onLoadEndListener) { + this.onLoadEndListener(route); + } + const module = factory.create(parentInjector); + // When loading a module that doesn't provide `RouterModule.forChild()` preloader + // will get stuck in an infinite loop. The child module's Injector will look to + // its parent `Injector` when it doesn't find any ROUTES so it will return routes + // for it's parent module instead. + return new LoadedRouterConfig( + flatten( + module.injector.get(ROUTES, undefined, InjectFlags.Self | InjectFlags.Optional)) + .map(standardizeConfig), + module); + }), + catchError((err) => { + route._loader$ = undefined; + throw err; + }), + ); + // Use custom ConnectableObservable as share in runners pipe increasing the bundle size too much + route._loader$ = new ConnectableObservable(loadRunner, () => new Subject()) + .pipe(refCount()); + return route._loader$; } private loadModuleFactory(loadChildren: LoadChildren): Observable> { diff --git a/packages/router/src/router_preloader.ts b/packages/router/src/router_preloader.ts index 81f4e17612..d110df6b51 100644 --- a/packages/router/src/router_preloader.ts +++ b/packages/router/src/router_preloader.ts @@ -126,7 +126,8 @@ export class RouterPreloader implements OnDestroy { private preloadConfig(ngModule: NgModuleRef, route: Route): Observable { return this.preloadingStrategy.preload(route, () => { - const loaded$ = this.loader.load(ngModule.injector, route); + const loaded$ = route._loadedConfig ? of(route._loadedConfig) : + this.loader.load(ngModule.injector, route); return loaded$.pipe(mergeMap((config: LoadedRouterConfig) => { route._loadedConfig = config; return this.processRoutes(config.module, config.routes); diff --git a/packages/router/test/apply_redirects.spec.ts b/packages/router/test/apply_redirects.spec.ts index 18dcf9718f..d7c32a9277 100644 --- a/packages/router/test/apply_redirects.spec.ts +++ b/packages/router/test/apply_redirects.spec.ts @@ -514,6 +514,45 @@ describe('applyRedirects', () => { expect(loaded).toEqual(['root', 'aux']); })); + it('should not try to load any matching configuration if previous load completed', + fakeAsync(() => { + const loadedConfig = + new LoadedRouterConfig([{path: 'a', component: ComponentA}], testModule); + let loadCalls = 0; + let loaded: string[] = []; + const loader = { + load: (injector: any, p: Route) => { + loadCalls++; + return of(loadedConfig) + .pipe( + delay(100 * loadCalls), + tap(() => loaded.push(p.loadChildren! as string)), + ); + } + }; + + const config: Routes = [ + {path: '**', loadChildren: 'children'}, + ]; + + applyRedirects(testModule.injector, loader, serializer, tree('xyz/a'), config) + .subscribe(); + expect(loadCalls).toBe(1); + tick(50); + expect(loaded).toEqual([]); + applyRedirects(testModule.injector, loader, serializer, tree('xyz/b'), config) + .subscribe(); + tick(50); + expect(loaded).toEqual(['children']); + expect(loadCalls).toBe(2); + tick(200); + applyRedirects(testModule.injector, loader, serializer, tree('xyz/c'), config) + .subscribe(); + tick(50); + expect(loadCalls).toBe(2); + tick(300); + })); + it('loads only the first match when two Routes with the same outlet have the same path', () => { const loadedConfig = new LoadedRouterConfig([{path: '', component: ComponentA}], testModule); let loadCalls = 0; diff --git a/packages/router/test/router_preloader.spec.ts b/packages/router/test/router_preloader.spec.ts index db4edfbd38..b1e4a82478 100644 --- a/packages/router/test/router_preloader.spec.ts +++ b/packages/router/test/router_preloader.spec.ts @@ -6,14 +6,18 @@ * found in the LICENSE file at https://angular.io/license */ -import {Compiler, Component, NgModule, NgModuleFactoryLoader, NgModuleRef} from '@angular/core'; +import {Compiler, Component, Injector, NgModule, NgModuleFactory, NgModuleFactoryLoader, NgModuleRef, Type} from '@angular/core'; +import {resolveComponentResources} from '@angular/core/src/metadata/resource_loading'; import {fakeAsync, inject, TestBed, tick} from '@angular/core/testing'; import {PreloadAllModules, PreloadingStrategy, RouterPreloader} from '@angular/router'; +import {BehaviorSubject, Observable, of, throwError} from 'rxjs'; +import {catchError, delay, filter, switchMap, take} from 'rxjs/operators'; import {Route, RouteConfigLoadEnd, RouteConfigLoadStart, Router, RouterModule} from '../index'; import {LoadedRouterConfig} from '../src/config'; import {RouterTestingModule, SpyNgModuleFactoryLoader} from '../testing'; + describe('RouterPreloader', () => { @Component({template: ''}) class LazyLoadedCmp { @@ -196,6 +200,311 @@ describe('RouterPreloader', () => { }))); }); + describe('should support preloading strategies', () => { + let delayLoadUnPaused: BehaviorSubject; + let delayLoadObserver$: Observable; + let events: Array; + + const subLoadChildrenSpy = jasmine.createSpy('submodule'); + const lazyLoadChildrenSpy = jasmine.createSpy('lazymodule'); + + const mockPreloaderFactory = (): PreloadingStrategy => { + class DelayedPreLoad implements PreloadingStrategy { + preload(route: Route, fn: () => Observable): Observable { + const routeName = + route.loadChildren ? (route.loadChildren as jasmine.Spy).and.identity : 'noChildren'; + return delayLoadObserver$.pipe( + filter(unpauseList => unpauseList.indexOf(routeName) !== -1), + take(1), + switchMap(() => { + return fn().pipe(catchError(() => of(null))); + }), + ); + } + } + return new DelayedPreLoad(); + }; + + @NgModule({ + declarations: [LazyLoadedCmp], + }) + class SharedModule { + } + + @NgModule({ + imports: [ + SharedModule, RouterModule.forChild([ + {path: 'LoadedModule1', component: LazyLoadedCmp}, + {path: 'sub', loadChildren: subLoadChildrenSpy} + ]) + ] + }) + class LoadedModule1 { + } + + @NgModule({ + imports: + [SharedModule, RouterModule.forChild([{path: 'LoadedModule2', component: LazyLoadedCmp}])] + }) + class LoadedModule2 { + } + + beforeEach(() => { + delayLoadUnPaused = new BehaviorSubject([]); + delayLoadObserver$ = delayLoadUnPaused.asObservable(); + subLoadChildrenSpy.calls.reset(); + lazyLoadChildrenSpy.calls.reset(); + TestBed.configureTestingModule({ + imports: + [RouterTestingModule.withRoutes([{path: 'lazy', loadChildren: lazyLoadChildrenSpy}])], + providers: [{provide: PreloadingStrategy, useFactory: mockPreloaderFactory}] + }); + events = []; + }); + + it('without reloading loaded modules', fakeAsync(() => { + const preloader = TestBed.inject(RouterPreloader); + const router = TestBed.inject(Router); + router.events.subscribe(e => { + if (e instanceof RouteConfigLoadEnd || e instanceof RouteConfigLoadStart) { + events.push(e); + } + }); + lazyLoadChildrenSpy.and.returnValue(of(LoadedModule1)); + + // App start activation of preloader + preloader.preload().subscribe((x) => {}); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(0); + + // Initial navigation cause route load + router.navigateByUrl('/lazy/LoadedModule1'); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(1); + + // Secondary load or navigation should use same loaded object ( + // ie this is a noop as the module should already be loaded) + delayLoadUnPaused.next(['lazymodule']); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(1); + expect(subLoadChildrenSpy).toHaveBeenCalledTimes(0); + expect(events.map(e => e.toString())).toEqual([ + 'RouteConfigLoadStart(path: lazy)', 'RouteConfigLoadEnd(path: lazy)' + ]); + })); + + it('and cope with the loader throwing exceptions during module load but allow retry', + fakeAsync(() => { + const preloader = TestBed.inject(RouterPreloader); + const router = TestBed.inject(Router); + router.events.subscribe(e => { + if (e instanceof RouteConfigLoadEnd || e instanceof RouteConfigLoadStart) { + events.push(e); + } + }); + + lazyLoadChildrenSpy.and.returnValue( + throwError('Error: Fake module load error (expectedreload)')); + preloader.preload().subscribe((x) => {}); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(0); + + delayLoadUnPaused.next(['lazymodule']); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(1); + + lazyLoadChildrenSpy.and.returnValue(of(LoadedModule1)); + router.navigateByUrl('/lazy/LoadedModule1').catch(() => { + fail('navigation should not throw'); + }); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(2); + expect(subLoadChildrenSpy).toHaveBeenCalledTimes(0); + + expect(events.map(e => e.toString())).toEqual([ + 'RouteConfigLoadStart(path: lazy)', 'RouteConfigLoadStart(path: lazy)', + 'RouteConfigLoadEnd(path: lazy)' + ]); + })); + + it('and cope with the loader throwing exceptions but allow retry', fakeAsync(() => { + const preloader = TestBed.inject(RouterPreloader); + const router = TestBed.inject(Router); + router.events.subscribe(e => { + if (e instanceof RouteConfigLoadEnd || e instanceof RouteConfigLoadStart) { + events.push(e); + } + }); + + lazyLoadChildrenSpy.and.returnValue( + throwError('Error: Fake module load error (expectedreload)')); + preloader.preload().subscribe((x) => {}); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(0); + + router.navigateByUrl('/lazy/LoadedModule1').catch((reason) => { + expect(reason).toEqual('Error: Fake module load error (expectedreload)'); + }); + tick(); + + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(1); + lazyLoadChildrenSpy.and.returnValue(of(LoadedModule1)); + router.navigateByUrl('/lazy/LoadedModule1').catch(() => { + fail('navigation should not throw'); + }); + tick(); + + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(2); + expect(subLoadChildrenSpy).toHaveBeenCalledTimes(0); + expect(events.map(e => e.toString())).toEqual([ + 'RouteConfigLoadStart(path: lazy)', 'RouteConfigLoadStart(path: lazy)', + 'RouteConfigLoadEnd(path: lazy)' + ]); + })); + + it('without autoloading loading submodules', fakeAsync(() => { + const preloader = TestBed.inject(RouterPreloader); + const router = TestBed.inject(Router); + router.events.subscribe(e => { + if (e instanceof RouteConfigLoadEnd || e instanceof RouteConfigLoadStart) { + events.push(e); + } + }); + + lazyLoadChildrenSpy.and.returnValue(of(LoadedModule1)); + subLoadChildrenSpy.and.returnValue(of(LoadedModule2)); + + preloader.preload().subscribe((x) => {}); + tick(); + router.navigateByUrl('/lazy/LoadedModule1'); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(1); + expect(subLoadChildrenSpy).toHaveBeenCalledTimes(0); + expect(events.map(e => e.toString())).toEqual([ + 'RouteConfigLoadStart(path: lazy)', 'RouteConfigLoadEnd(path: lazy)' + ]); + + // Release submodule to check it does in fact load + delayLoadUnPaused.next(['lazymodule', 'submodule']); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(1); + expect(subLoadChildrenSpy).toHaveBeenCalledTimes(1); + expect(events.map(e => e.toString())).toEqual([ + 'RouteConfigLoadStart(path: lazy)', 'RouteConfigLoadEnd(path: lazy)', + 'RouteConfigLoadStart(path: sub)', 'RouteConfigLoadEnd(path: sub)' + ]); + })); + + it('and close the preload obsservable ', fakeAsync(() => { + const preloader = TestBed.inject(RouterPreloader); + const router = TestBed.inject(Router); + router.events.subscribe(e => { + if (e instanceof RouteConfigLoadEnd || e instanceof RouteConfigLoadStart) { + events.push(e); + } + }); + + lazyLoadChildrenSpy.and.returnValue(of(LoadedModule1)); + subLoadChildrenSpy.and.returnValue(of(LoadedModule2)); + const preloadSubscription = preloader.preload().subscribe((x) => {}); + + router.navigateByUrl('/lazy/LoadedModule1'); + tick(); + delayLoadUnPaused.next(['lazymodule', 'submodule']); + tick(); + + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(1); + expect(subLoadChildrenSpy).toHaveBeenCalledTimes(1); + expect(preloadSubscription.closed).toBeTruthy(); + })); + + it('with overlapping loads from navigation and the preloader', fakeAsync(() => { + const preloader = TestBed.inject(RouterPreloader); + const router = TestBed.inject(Router); + router.events.subscribe(e => { + if (e instanceof RouteConfigLoadEnd || e instanceof RouteConfigLoadStart) { + events.push(e); + } + }); + + lazyLoadChildrenSpy.and.returnValue(of(LoadedModule1)); + subLoadChildrenSpy.and.returnValue(of(LoadedModule2).pipe(delay(5))); + preloader.preload().subscribe((x) => {}); + tick(); + + // Load the out modules at start of test and ensure it and only + // it is loaded + delayLoadUnPaused.next(['lazymodule']); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(1); + expect(events.map(e => e.toString())).toEqual([ + 'RouteConfigLoadStart(path: lazy)', + 'RouteConfigLoadEnd(path: lazy)', + ]); + + // Cause the load from router to start (has 5 tick delay) + router.navigateByUrl('/lazy/sub/LoadedModule2'); + tick(); // T1 + // Cause the load from preloader to start + delayLoadUnPaused.next(['lazymodule', 'submodule']); + tick(); // T2 + + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(1); + expect(subLoadChildrenSpy).toHaveBeenCalledTimes(1); + tick(5); // T2 to T7 enough time for mutiple loads to finish + + expect(subLoadChildrenSpy).toHaveBeenCalledTimes(1); + expect(events.map(e => e.toString())).toEqual([ + 'RouteConfigLoadStart(path: lazy)', 'RouteConfigLoadEnd(path: lazy)', + 'RouteConfigLoadStart(path: sub)', 'RouteConfigLoadEnd(path: sub)' + ]); + })); + + it('cope with factory fail from broken modules', fakeAsync(() => { + const preloader = TestBed.inject(RouterPreloader); + const router = TestBed.inject(Router); + router.events.subscribe(e => { + if (e instanceof RouteConfigLoadEnd || e instanceof RouteConfigLoadStart) { + events.push(e); + } + }); + + class BrokenModuleFactory extends NgModuleFactory { + moduleType: Type = LoadedModule1; + constructor() { + super(); + } + create(_parentInjector: Injector|null): NgModuleRef { + throw 'Error: Broken module'; + } + } + + lazyLoadChildrenSpy.and.returnValue(of(new BrokenModuleFactory())); + preloader.preload().subscribe((x) => {}); + tick(); + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(0); + + router.navigateByUrl('/lazy/LoadedModule1').catch((reason) => { + expect(reason).toEqual('Error: Broken module'); + }); + tick(); + + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(1); + lazyLoadChildrenSpy.and.returnValue(of(LoadedModule1)); + router.navigateByUrl('/lazy/LoadedModule1').catch(() => { + fail('navigation should not throw'); + }); + tick(); + + expect(lazyLoadChildrenSpy).toHaveBeenCalledTimes(2); + expect(subLoadChildrenSpy).toHaveBeenCalledTimes(0); + expect(events.map(e => e.toString())).toEqual([ + 'RouteConfigLoadStart(path: lazy)', 'RouteConfigLoadEnd(path: lazy)', + 'RouteConfigLoadStart(path: lazy)', 'RouteConfigLoadEnd(path: lazy)' + ]); + })); + }); + describe('should ignore errors', () => { @NgModule({ declarations: [LazyLoadedCmp],