From 8f24bc9443b3872fe095d9f7f77b308a361a13b4 Mon Sep 17 00:00:00 2001 From: Misko Hevery Date: Wed, 19 Aug 2020 21:05:31 -0700 Subject: [PATCH] Revert "fix(router): support lazy loading for empty path named outlets (#38379)" This reverts commit 7ad32649c0d0004fcc3604c62cf0c1ae159a825b. --- .../size-tracking/integration-payloads.json | 2 +- packages/router/src/apply_redirects.ts | 69 ++++------ packages/router/test/apply_redirects.spec.ts | 127 +----------------- 3 files changed, 27 insertions(+), 171 deletions(-) diff --git a/goldens/size-tracking/integration-payloads.json b/goldens/size-tracking/integration-payloads.json index b6c8981eb9..2d5ce83c09 100644 --- a/goldens/size-tracking/integration-payloads.json +++ b/goldens/size-tracking/integration-payloads.json @@ -39,7 +39,7 @@ "master": { "uncompressed": { "runtime-es2015": 2289, - "main-es2015": 245885, + "main-es2015": 245351, "polyfills-es2015": 36938, "5-es2015": 751 } diff --git a/packages/router/src/apply_redirects.ts b/packages/router/src/apply_redirects.ts index 21b7b00234..5eb4050473 100644 --- a/packages/router/src/apply_redirects.ts +++ b/packages/router/src/apply_redirects.ts @@ -8,7 +8,7 @@ import {Injector, NgModuleRef} from '@angular/core'; import {defer, EmptyError, Observable, Observer, of} from 'rxjs'; -import {catchError, first, map, mergeMap, switchMap, tap} from 'rxjs/operators'; +import {catchError, concatAll, first, map, mergeMap, tap} from 'rxjs/operators'; import {LoadedRouterConfig, Route, Routes} from './config'; import {CanLoadFn} from './interfaces'; @@ -148,47 +148,28 @@ class ApplyRedirects { ngModule: NgModuleRef, segmentGroup: UrlSegmentGroup, routes: Route[], segments: UrlSegment[], outlet: string, allowRedirects: boolean): Observable { - type MatchedSegment = {segment: UrlSegmentGroup, outlet: string}; - // This logic takes each route and switches to a new observable that depends on the result of - // the previous route expansion. In this way, we compose a list of results where each one can - // depend on and look at the previous to determine how to proceed with expansion of the - // current route. - return routes - .reduce( - (accumulatedResults: Observable>, r: Route) => { - return accumulatedResults.pipe(switchMap(resultsThusFar => { - // If we already matched a previous `Route` with the same outlet as the current, - // we should not process the current one. - if (resultsThusFar.some(result => result && result.outlet === getOutlet(r))) { - return of(resultsThusFar); - } - const expanded$ = this.expandSegmentAgainstRoute( - ngModule, segmentGroup, routes, r, segments, outlet, allowRedirects); - return expanded$.pipe( - map((segment) => resultsThusFar.concat({segment, outlet: getOutlet(r)})), - catchError((e: any) => { - if (e instanceof NoMatch) { - return of(resultsThusFar); - } - throw e; - })); - })); - }, - of([] as MatchedSegment[])) - .pipe( - // Find the matched segment whose outlet matches the one we're looking for. - map(results => results.find(s => s.outlet === outlet)?.segment), - first((s): s is UrlSegmentGroup => s !== undefined), - catchError((e: any, _: any) => { - if (e instanceof EmptyError || e.name === 'EmptyError') { - if (this.noLeftoversInUrl(segmentGroup, segments, outlet)) { - return of(new UrlSegmentGroup([], {})); - } - throw new NoMatch(segmentGroup); - } - throw e; - }), - ); + return of(...routes).pipe( + map((r: any) => { + const expanded$ = this.expandSegmentAgainstRoute( + ngModule, segmentGroup, routes, r, segments, outlet, allowRedirects); + return expanded$.pipe(catchError((e: any) => { + if (e instanceof NoMatch) { + // TODO(i): this return type doesn't match the declared Observable - + // talk to Jason + return of(null) as any; + } + throw e; + })); + }), + concatAll(), first((s: any) => !!s), catchError((e: any, _: any) => { + if (e instanceof EmptyError || e.name === 'EmptyError') { + if (this.noLeftoversInUrl(segmentGroup, segments, outlet)) { + return of(new UrlSegmentGroup([], {})); + } + throw new NoMatch(segmentGroup); + } + throw e; + })); } private noLeftoversInUrl(segmentGroup: UrlSegmentGroup, segments: UrlSegment[], outlet: string): @@ -199,9 +180,7 @@ class ApplyRedirects { private expandSegmentAgainstRoute( ngModule: NgModuleRef, segmentGroup: UrlSegmentGroup, routes: Route[], route: Route, paths: UrlSegment[], outlet: string, allowRedirects: boolean): Observable { - // Empty string segments are special because multiple outlets can match a single path, i.e. - // `[{path: '', component: B}, {path: '', loadChildren: () => {}, outlet: "about"}]` - if (getOutlet(route) !== outlet && route.path !== '') { + if (getOutlet(route) !== outlet) { return noMatch(segmentGroup); } diff --git a/packages/router/test/apply_redirects.spec.ts b/packages/router/test/apply_redirects.spec.ts index 812b7b83bb..d564727a88 100644 --- a/packages/router/test/apply_redirects.spec.ts +++ b/packages/router/test/apply_redirects.spec.ts @@ -7,9 +7,9 @@ */ import {NgModuleRef} from '@angular/core'; -import {fakeAsync, TestBed, tick} from '@angular/core/testing'; +import {TestBed} from '@angular/core/testing'; import {Observable, of} from 'rxjs'; -import {delay, tap} from 'rxjs/operators'; +import {delay} from 'rxjs/operators'; import {applyRedirects} from '../src/apply_redirects'; import {LoadedRouterConfig, Route, Routes} from '../src/config'; @@ -482,89 +482,6 @@ describe('applyRedirects', () => { expect((config[0] as any)._loadedConfig).toBe(loadedConfig); }); }); - - it('should load all matching configurations of empty path, including an auxiliary outlets', - fakeAsync(() => { - const loadedConfig = - new LoadedRouterConfig([{path: '', 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: 'root'}, {path: '', loadChildren: 'aux', outlet: 'popup'}]; - - applyRedirects(testModule.injector, loader, serializer, tree(''), config).subscribe(); - expect(loadCalls).toBe(1); - tick(100); - expect(loaded).toEqual(['root']); - tick(200); - expect(loadCalls).toBe(2); - expect(loaded).toEqual(['root', 'aux']); - })); - - 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; - let loaded: string[] = []; - const loader = { - load: (injector: any, p: Route) => { - loadCalls++; - return of(loadedConfig) - .pipe( - tap(() => loaded.push(p.loadChildren! as string)), - ); - } - }; - - const config: Routes = - [{path: 'a', loadChildren: 'first'}, {path: 'a', loadChildren: 'second'}]; - - applyRedirects(testModule.injector, loader, serializer, tree('a'), config).subscribe(); - expect(loadCalls).toBe(1); - expect(loaded).toEqual(['first']); - }); - - it('should load the configuration of empty root path if the entry is an aux outlet', - fakeAsync(() => { - const loadedConfig = - new LoadedRouterConfig([{path: '', component: ComponentA}], testModule); - let loaded: string[] = []; - const rootDelay = 100; - const auxDelay = 1; - const loader = { - load: (injector: any, p: Route) => { - const delayMs = p.loadChildren! as string === 'aux' ? auxDelay : rootDelay; - return of(loadedConfig) - .pipe( - delay(delayMs), - tap(() => loaded.push(p.loadChildren! as string)), - ); - } - }; - - const config: Routes = [ - // Define aux route first so it matches before the primary outlet - {path: 'modal', loadChildren: 'aux', outlet: 'popup'}, - {path: '', loadChildren: 'root'}, - ]; - - applyRedirects(testModule.injector, loader, serializer, tree('(popup:modal)'), config) - .subscribe(); - tick(auxDelay); - expect(loaded).toEqual(['aux']); - tick(rootDelay); - expect(loaded).toEqual(['aux', 'root']); - })); }); describe('empty paths', () => { @@ -837,46 +754,6 @@ describe('applyRedirects', () => { }); }); - describe('multiple matches with empty path named outlets', () => { - it('should work with redirects when other outlet comes before the one being activated', () => { - applyRedirects( - testModule.injector, null!, serializer, tree(''), - [ - { - path: '', - children: [ - {path: '', component: ComponentA, outlet: 'aux'}, - {path: '', redirectTo: 'b', pathMatch: 'full'}, - {path: 'b', component: ComponentB}, - ], - }, - ]) - .subscribe( - (tree: UrlTree) => { - expect(tree.toString()).toEqual('/b'); - }, - () => { - fail('should not be reached'); - }); - }); - - it('should work when entry point is named outlet', () => { - applyRedirects( - testModule.injector, null!, serializer, tree('(popup:modal)'), - [ - {path: '', component: ComponentA}, - {path: 'modal', component: ComponentB, outlet: 'popup'}, - ]) - .subscribe( - (tree: UrlTree) => { - expect(tree.toString()).toEqual('/(popup:modal)'); - }, - (e) => { - fail('should not be reached' + e.message); - }); - }); - }); - describe('redirecting to named outlets', () => { it('should work when using absolute redirects', () => { checkRedirect(