From 267c566baf0bc1998f277b3fc4b59152784fe87a Mon Sep 17 00:00:00 2001 From: Joey Perrott Date: Thu, 11 Feb 2021 11:19:57 -0800 Subject: [PATCH] Revert "fix(router): fix load interaction of navigation and preload strategies (#40389)" (#40806) This reverts commit e9a19a6152cc742f1617ee696202a7fbd06fff3d. PR Close #40806 --- 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, 31 insertions(+), 398 deletions(-) diff --git a/goldens/size-tracking/aio-payloads.json b/goldens/size-tracking/aio-payloads.json index d42dee64fe..0520b53e03 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": 448036, + "main-es2015": 447514, "polyfills-es2015": 52493 } } @@ -21,9 +21,9 @@ "master": { "uncompressed": { "runtime-es2015": 3153, - "main-es2015": 432647, + "main-es2015": 432078, "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 aba3add9a9..3a456cfc71 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": 241843, + "main-es2015": 241202, "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 f908d45c66..98f195099c 100644 --- a/packages/router/src/apply_redirects.ts +++ b/packages/router/src/apply_redirects.ts @@ -280,12 +280,11 @@ class ApplyRedirects { segments: UrlSegment[], outlet: string): Observable { if (route.path === '**') { if (route.loadChildren) { - 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 this.configLoader.load(ngModule.injector, route) + .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 fbfd9a7f5d..86af574f02 100644 --- a/packages/router/src/config.ts +++ b/packages/router/src/config.ts @@ -483,11 +483,6 @@ 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 0810b785a9..e9d21974ee 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 {ConnectableObservable, from, Observable, of, Subject} from 'rxjs'; -import {catchError, map, mergeMap, refCount, tap} from 'rxjs/operators'; +import {from, Observable, of} from 'rxjs'; +import {map, mergeMap} from 'rxjs/operators'; import {LoadChildren, LoadedRouterConfig, Route} from './config'; import {flatten, wrapIntoObservable} from './utils/collection'; @@ -28,39 +28,27 @@ 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!); - 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$; + + 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); + })); } private loadModuleFactory(loadChildren: LoadChildren): Observable> { diff --git a/packages/router/src/router_preloader.ts b/packages/router/src/router_preloader.ts index d110df6b51..81f4e17612 100644 --- a/packages/router/src/router_preloader.ts +++ b/packages/router/src/router_preloader.ts @@ -126,8 +126,7 @@ export class RouterPreloader implements OnDestroy { private preloadConfig(ngModule: NgModuleRef, route: Route): Observable { return this.preloadingStrategy.preload(route, () => { - const loaded$ = route._loadedConfig ? of(route._loadedConfig) : - this.loader.load(ngModule.injector, route); + const loaded$ = 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 d7c32a9277..18dcf9718f 100644 --- a/packages/router/test/apply_redirects.spec.ts +++ b/packages/router/test/apply_redirects.spec.ts @@ -514,45 +514,6 @@ 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 b1e4a82478..db4edfbd38 100644 --- a/packages/router/test/router_preloader.spec.ts +++ b/packages/router/test/router_preloader.spec.ts @@ -6,18 +6,14 @@ * found in the LICENSE file at https://angular.io/license */ -import {Compiler, Component, Injector, NgModule, NgModuleFactory, NgModuleFactoryLoader, NgModuleRef, Type} from '@angular/core'; -import {resolveComponentResources} from '@angular/core/src/metadata/resource_loading'; +import {Compiler, Component, NgModule, NgModuleFactoryLoader, NgModuleRef} from '@angular/core'; 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 { @@ -200,311 +196,6 @@ 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],