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

@@ -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
       }
     }
   }
-}
+}

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 @@
       }
     }
   }
-}
+}

packages/router/src/apply_redirects.ts

@@ -280,11 +280,12 @@ class ApplyRedirects {
       segments: UrlSegment[], outlet: string): Observable<UrlSegmentGroup> {
     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, {}));

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<LoadedRouterConfig>;
 }
 
 export class LoadedRouterConfig {

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<LoadedRouterConfig> {
+    if (route._loader$) {
+      return route._loader$;
+    }
+
     if (this.onLoadStartListener) {
       this.onLoadStartListener(route);
     }
-
     const moduleFactory$ = this.loadModuleFactory(route.loadChildren!);
-
-    return moduleFactory$.pipe(map((factory: NgModuleFactory<any>) => {
-      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<any>) => {
+          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<LoadedRouterConfig>())
+                         .pipe(refCount());
+    return route._loader$;
   }
 
   private loadModuleFactory(loadChildren: LoadChildren): Observable<NgModuleFactory<any>> {

packages/router/src/router_preloader.ts

@@ -126,7 +126,8 @@ export class RouterPreloader implements OnDestroy {
 
   private preloadConfig(ngModule: NgModuleRef<any>, route: Route): Observable<void> {
     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);

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, <any>loader, serializer, tree('xyz/a'), config)
+             .subscribe();
+         expect(loadCalls).toBe(1);
+         tick(50);
+         expect(loaded).toEqual([]);
+         applyRedirects(testModule.injector, <any>loader, serializer, tree('xyz/b'), config)
+             .subscribe();
+         tick(50);
+         expect(loaded).toEqual(['children']);
+         expect(loadCalls).toBe(2);
+         tick(200);
+         applyRedirects(testModule.injector, <any>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;

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<string[]>;
+    let delayLoadObserver$: Observable<string[]>;
+    let events: Array<RouteConfigLoadStart|RouteConfigLoadEnd>;
+
+    const subLoadChildrenSpy = jasmine.createSpy('submodule');
+    const lazyLoadChildrenSpy = jasmine.createSpy('lazymodule');
+
+    const mockPreloaderFactory = (): PreloadingStrategy => {
+      class DelayedPreLoad implements PreloadingStrategy {
+        preload(route: Route, fn: () => Observable<any>): Observable<any> {
+          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<string[]>([]);
+      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<any> {
+           moduleType: Type<any> = LoadedModule1;
+           constructor() {
+             super();
+           }
+           create(_parentInjector: Injector|null): NgModuleRef<any> {
+             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],