Skip to content

Commit

Permalink
fix(router): support lazy loading for empty path named outlets (#38379)
Browse files Browse the repository at this point in the history
In general, the router only matches and loads a single Route config tree. However,
named outlets with empty paths are a special case where the router can
and should actually match two different `Route`s and ensure that the
modules are loaded for each match.

This change updates the "ApplyRedirects" stage to ensure that named
outlets with empty paths finish loading their configs before proceeding
to the next stage in the routing pipe. This is necessary because if the
named outlet has `loadChildren` but the associated lazy config is not loaded
before following stages attempt to match and activate relevant `Route`s,
an error will occur.

fixes #12842

PR Close #38379
  • Loading branch information
atscott committed Sep 8, 2020
1 parent 97475d7 commit 926ffcd
Show file tree
Hide file tree
Showing 4 changed files with 206 additions and 31 deletions.
4 changes: 2 additions & 2 deletions goldens/size-tracking/integration-payloads.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2289,
"main-es2015": 245351,
"main-es2015": 245885,
"polyfills-es2015": 36938,
"5-es2015": 751
}
Expand All @@ -49,7 +49,7 @@
"master": {
"uncompressed": {
"runtime-es2015": 2289,
"main-es2015": 221939,
"main-es2015": 222476,
"polyfills-es2015": 36723,
"5-es2015": 781
}
Expand Down
77 changes: 50 additions & 27 deletions packages/router/src/apply_redirects.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
*/

import {Injector, NgModuleRef} from '@angular/core';
import {defer, EmptyError, Observable, Observer, of} from 'rxjs';
import {catchError, concatAll, first, map, mergeMap, tap} from 'rxjs/operators';
import {defer, EmptyError, from, Observable, Observer, of} from 'rxjs';
import {catchError, combineAll, concatMap, first, map, mergeMap, tap} from 'rxjs/operators';

import {LoadedRouterConfig, Route, Routes} from './config';
import {CanLoadFn} from './interfaces';
Expand All @@ -17,6 +17,7 @@ import {RouterConfigLoader} from './router_config_loader';
import {defaultUrlMatcher, navigationCancelingError, Params, PRIMARY_OUTLET} from './shared';
import {UrlSegment, UrlSegmentGroup, UrlSerializer, UrlTree} from './url_tree';
import {forEach, waitForMap, wrapIntoObservable} from './utils/collection';
import {getOutlet, groupRoutesByOutlet} from './utils/config';
import {isCanLoad, isFunction, isUrlTree} from './utils/type_guards';

class NoMatch {
Expand Down Expand Up @@ -148,28 +149,52 @@ class ApplyRedirects {
ngModule: NgModuleRef<any>, segmentGroup: UrlSegmentGroup, routes: Route[],
segments: UrlSegment[], outlet: string,
allowRedirects: boolean): Observable<UrlSegmentGroup> {
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<UrlSegmentGroup> -
// talk to Jason
return of(null) as any;
// We need to expand each outlet group independently to ensure that we not only load modules
// for routes matching the given `outlet`, but also those which will be activated because
// their path is empty string. This can result in multiple outlets being activated at once.
const routesByOutlet: Map<string, Route[]> = groupRoutesByOutlet(routes);
if (!routesByOutlet.has(outlet)) {
routesByOutlet.set(outlet, []);
}

const expandRoutes = (routes: Route[]) => {
return from(routes).pipe(
concatMap((r: Route) => {
const expanded$ = this.expandSegmentAgainstRoute(
ngModule, segmentGroup, routes, r, segments, outlet, allowRedirects);
return expanded$.pipe(catchError(e => {
if (e instanceof NoMatch) {
return of(null);
}
throw e;
}));
}),
first((s: UrlSegmentGroup|null): s is UrlSegmentGroup => s !== null),
catchError(e => {
if (e instanceof EmptyError || e.name === 'EmptyError') {
if (this.noLeftoversInUrl(segmentGroup, segments, outlet)) {
return of(new UrlSegmentGroup([], {}));
}
throw new NoMatch(segmentGroup);
}
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;
}));
}),
);
};

const expansions = Array.from(routesByOutlet.entries()).map(([routeOutlet, routes]) => {
const expanded = expandRoutes(routes);
// Map all results from outlets we aren't activating to `null` so they can be ignored later
return routeOutlet === outlet ? expanded :
expanded.pipe(map(() => null), catchError(() => of(null)));
});
return from(expansions)
.pipe(
combineAll(),
first(),
// Return only the expansion for the route outlet we are trying to activate.
map(results => results.find(result => result !== null)!),
);
}

private noLeftoversInUrl(segmentGroup: UrlSegmentGroup, segments: UrlSegment[], outlet: string):
Expand All @@ -180,7 +205,9 @@ class ApplyRedirects {
private expandSegmentAgainstRoute(
ngModule: NgModuleRef<any>, segmentGroup: UrlSegmentGroup, routes: Route[], route: Route,
paths: UrlSegment[], outlet: string, allowRedirects: boolean): Observable<UrlSegmentGroup> {
if (getOutlet(route) !== outlet) {
// 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 !== '') {
return noMatch(segmentGroup);
}

Expand Down Expand Up @@ -551,7 +578,3 @@ function isEmptyPathRedirect(

return r.path === '' && r.redirectTo !== undefined;
}

function getOutlet(route: Route): string {
return route.outlet || PRIMARY_OUTLET;
}
18 changes: 18 additions & 0 deletions packages/router/src/utils/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,3 +113,21 @@ export function standardizeConfig(r: Route): Route {
}
return c;
}

/** Returns of `Map` of outlet names to the `Route`s for that outlet. */
export function groupRoutesByOutlet(routes: Route[]): Map<string, Route[]> {
return routes.reduce((map, route) => {
const routeOutlet = getOutlet(route);
if (map.has(routeOutlet)) {
map.get(routeOutlet)!.push(route);
} else {
map.set(routeOutlet, [route]);
}
return map;
}, new Map<string, Route[]>());
}

/** Returns the `route.outlet` or PRIMARY_OUTLET if none exists. */
export function getOutlet(route: Route): string {
return route.outlet || PRIMARY_OUTLET;
}
138 changes: 136 additions & 2 deletions packages/router/test/apply_redirects.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@
*/

import {NgModuleRef} from '@angular/core';
import {TestBed} from '@angular/core/testing';
import {fakeAsync, TestBed, tick} from '@angular/core/testing';
import {Observable, of} from 'rxjs';
import {delay} from 'rxjs/operators';
import {delay, tap} from 'rxjs/operators';

import {applyRedirects} from '../src/apply_redirects';
import {LoadedRouterConfig, Route, Routes} from '../src/config';
Expand Down Expand Up @@ -482,6 +482,88 @@ 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, <any>loader, serializer, tree(''), config).subscribe();
expect(loadCalls).toBe(2);
tick(100);
expect(loaded).toEqual(['root']);
tick(100);
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, <any>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, <any>loader, serializer, tree('(popup:modal)'), config)
.subscribe();
tick(auxDelay);
expect(loaded).toEqual(['aux']);
tick(rootDelay);
expect(loaded).toEqual(['aux', 'root']);
}));
});

describe('empty paths', () => {
Expand Down Expand Up @@ -754,6 +836,46 @@ 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(
Expand Down Expand Up @@ -794,6 +916,18 @@ describe('applyRedirects', () => {
});
});
});

// internal failure b/165719418
it('does not fail with large configs', () => {
const config: Routes = [];
for (let i = 0; i < 400; i++) {
config.push({path: 'no_match', component: ComponentB});
}
config.push({path: 'match', component: ComponentA});
applyRedirects(testModule.injector, null!, serializer, tree('match'), config).forEach(r => {
expectTreeToBe(r, 'match');
});
});
});

function checkRedirect(config: Routes, url: string, callback: any): void {
Expand Down

3 comments on commit 926ffcd

@arpansac
Copy link

@arpansac arpansac commented on 926ffcd Nov 19, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @atscott
I spent the last night going through a good number of links while trying it out with the new Angular, need help :D

This is inside a lazy loaded module. If I replace path: '' with path: 'asdf' it works.
The router-outlet for 'popup' lies in CommunityChannelsDashboardComponent

    component: CommunityChannelsDashboardComponent,
    resolve: {
      community: CommunityDetailsResolver
    },
    children: [
      {
        path: 'new-channel',
        outlet: 'popup',
        component: CommunityChannelFormComponent,
      },
      {
        path: ':community_channel_id',
        component: CommunityChannelDiscussionComponent,
        resolve: {
          // community: CommunityChannelResolver
        },
        children: [
          {
            path: 'edit',
            component: CommunityChannelFormComponent
          },
          {
            path: 'settings',
            component: ChannelSettingsComponent
          }
        ]
      },
    ],```

Thanks!

@atscott
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @arpansac, this looks like #10726. There’s no solution for this at the moment other than not using an empty path for the parent of the named outlet.

@arpansac
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @atscott for the update, I'll continue the conversation there!

Please sign in to comment.