Skip to content

Commit f542e4e

Browse files
atscottjosephperrott
atscott
authored and
josephperrott
committed
fix(router): apply redirects should match named outlets with empty path parents (#40029) (#40315)
There are two parts to this commit: 1. Revert the changes from #38379. This change had an incomplete view of how things worked and also diverged the implementations of `applyRedirects` and `recognize` even more. 2. Apply the fixes from the `recognize` algorithm to ensure that named outlets with empty path parents can be matched. This change also passes all the tests that were added in #38379 with the added benefit of being a more complete fix that stays in-line with the `recognize` algorithm. This was made possible by using the same approach for `split` by always creating segments for empty path matches (previously, this was only done in `applyRedirects` if there was a `redirectTo` value). At the end of the expansions, we need to squash all empty segments so that serializing the final `UrlTree` returns the same result as before. Fixes #39952 Fixes #10726 Closes #30410 PR Close #40029 PR Close #40315
1 parent 68a9511 commit f542e4e

File tree

9 files changed

+280
-138
lines changed

9 files changed

+280
-138
lines changed

‎goldens/size-tracking/integration-payloads.json‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@
3939
"master": {
4040
"uncompressed": {
4141
"runtime-es2015": 2285,
42-
"main-es2015": 241457,
42+
"main-es2015": 240836,
4343
"polyfills-es2015": 36709,
4444
"5-es2015": 745
4545
}
@@ -49,7 +49,7 @@
4949
"master": {
5050
"uncompressed": {
5151
"runtime-es2015": 2285,
52-
"main-es2015": 218340,
52+
"main-es2015": 217616,
5353
"polyfills-es2015": 36709,
5454
"5-es2015": 777
5555
}
@@ -66,4 +66,4 @@
6666
}
6767
}
6868
}
69-
}
69+
}

‎packages/core/test/bundling/router/bundle.golden_symbols.json‎

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1451,6 +1451,9 @@
14511451
{
14521452
"name": "handleError"
14531453
},
1454+
{
1455+
"name": "hasEmptyPathConfig"
1456+
},
14541457
{
14551458
"name": "hasParentInjector"
14561459
},
@@ -1536,13 +1539,13 @@
15361539
"name": "isDirectiveHost"
15371540
},
15381541
{
1539-
"name": "isEmptyPathRedirect"
1542+
"name": "isFunction"
15401543
},
15411544
{
15421545
"name": "isFunction"
15431546
},
15441547
{
1545-
"name": "isFunction"
1548+
"name": "isImmediateMatch"
15461549
},
15471550
{
15481551
"name": "isInCheckNoChangesMode"
@@ -1640,9 +1643,6 @@
16401643
{
16411644
"name": "map"
16421645
},
1643-
{
1644-
"name": "mapChildrenIntoArray"
1645-
},
16461646
{
16471647
"name": "markAsComponentHost"
16481648
},
@@ -1676,9 +1676,6 @@
16761676
{
16771677
"name": "mergeMap"
16781678
},
1679-
{
1680-
"name": "mergeTrivialChildren"
1681-
},
16821679
{
16831680
"name": "modules"
16841681
},
@@ -1700,6 +1697,9 @@
17001697
{
17011698
"name": "navigationCancelingError"
17021699
},
1700+
{
1701+
"name": "newObservableError"
1702+
},
17031703
{
17041704
"name": "nextBindingIndex"
17051705
},
@@ -1709,6 +1709,12 @@
17091709
{
17101710
"name": "ngOnChangesSetInput"
17111711
},
1712+
{
1713+
"name": "noLeftoversInUrl"
1714+
},
1715+
{
1716+
"name": "noMatch"
1717+
},
17121718
{
17131719
"name": "noMatch"
17141720
},
@@ -1832,6 +1838,9 @@
18321838
{
18331839
"name": "saveNameToExportMap"
18341840
},
1841+
{
1842+
"name": "scan"
1843+
},
18351844
{
18361845
"name": "scheduleArray"
18371846
},
@@ -1904,9 +1913,15 @@
19041913
{
19051914
"name": "shouldSearchParent"
19061915
},
1916+
{
1917+
"name": "sortByMatchingOutlets"
1918+
},
19071919
{
19081920
"name": "split"
19091921
},
1922+
{
1923+
"name": "squashSegmentGroup"
1924+
},
19101925
{
19111926
"name": "standardizeConfig"
19121927
},

‎packages/router/src/apply_redirects.ts‎

Lines changed: 72 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -8,17 +8,17 @@
88

99
import {Injector, NgModuleRef} from '@angular/core';
1010
import {EmptyError, from, Observable, Observer, of} from 'rxjs';
11-
import {catchError, combineAll, concatMap, first, map, mergeMap, tap} from 'rxjs/operators';
11+
import {catchError, concatMap, first, last, map, mergeMap, scan, tap} from 'rxjs/operators';
1212

1313
import {LoadedRouterConfig, Route, Routes} from './config';
1414
import {CanLoadFn} from './interfaces';
1515
import {prioritizedGuardValue} from './operators/prioritized_guard_value';
1616
import {RouterConfigLoader} from './router_config_loader';
1717
import {navigationCancelingError, Params, PRIMARY_OUTLET} from './shared';
1818
import {UrlSegment, UrlSegmentGroup, UrlSerializer, UrlTree} from './url_tree';
19-
import {forEach, waitForMap, wrapIntoObservable} from './utils/collection';
20-
import {getOutlet, groupRoutesByOutlet} from './utils/config';
21-
import {match, noLeftoversInUrl, split} from './utils/config_matching';
19+
import {forEach, wrapIntoObservable} from './utils/collection';
20+
import {getOutlet, sortByMatchingOutlets} from './utils/config';
21+
import {isImmediateMatch, match, noLeftoversInUrl, split} from './utils/config_matching';
2222
import {isCanLoad, isFunction, isUrlTree} from './utils/type_guards';
2323

2424
class NoMatch {
@@ -78,8 +78,17 @@ class ApplyRedirects {
7878
}
7979

8080
apply(): Observable<UrlTree> {
81+
const splitGroup = split(this.urlTree.root, [], [], this.config).segmentGroup;
82+
// TODO(atscott): creating a new segment removes the _sourceSegment _segmentIndexShift, which is
83+
// only necessary to prevent failures in tests which assert exact object matches. The `split` is
84+
// now shared between `applyRedirects` and `recognize` but only the `recognize` step needs these
85+
// properties. Before the implementations were merged, the `applyRedirects` would not assign
86+
// them. We should be able to remove this logic as a "breaking change" but should do some more
87+
// investigation into the failures first.
88+
const rootSegmentGroup = new UrlSegmentGroup(splitGroup.segments, splitGroup.children);
89+
8190
const expanded$ =
82-
this.expandSegmentGroup(this.ngModule, this.config, this.urlTree.root, PRIMARY_OUTLET);
91+
this.expandSegmentGroup(this.ngModule, this.config, rootSegmentGroup, PRIMARY_OUTLET);
8392
const urlTrees$ = expanded$.pipe(map((rootSegmentGroup: UrlSegmentGroup) => {
8493
return this.createUrlTree(
8594
squashSegmentGroup(rootSegmentGroup), this.urlTree.queryParams, this.urlTree.fragment!);
@@ -143,74 +152,73 @@ class ApplyRedirects {
143152
private expandChildren(
144153
ngModule: NgModuleRef<any>, routes: Route[],
145154
segmentGroup: UrlSegmentGroup): Observable<{[name: string]: UrlSegmentGroup}> {
146-
return waitForMap(
147-
segmentGroup.children,
148-
(childOutlet, child) => this.expandSegmentGroup(ngModule, routes, child, childOutlet));
155+
// Expand outlets one at a time, starting with the primary outlet. We need to do it this way
156+
// because an absolute redirect from the primary outlet takes precedence.
157+
const childOutlets: string[] = [];
158+
for (const child of Object.keys(segmentGroup.children)) {
159+
if (child === 'primary') {
160+
childOutlets.unshift(child);
161+
} else {
162+
childOutlets.push(child);
163+
}
164+
}
165+
166+
return from(childOutlets)
167+
.pipe(
168+
concatMap(childOutlet => {
169+
const child = segmentGroup.children[childOutlet];
170+
// Sort the routes so routes with outlets that match the the segment appear
171+
// first, followed by routes for other outlets, which might match if they have an
172+
// empty path.
173+
const sortedRoutes = sortByMatchingOutlets(routes, childOutlet);
174+
return this.expandSegmentGroup(ngModule, sortedRoutes, child, childOutlet)
175+
.pipe(map(s => ({segment: s, outlet: childOutlet})));
176+
}),
177+
scan(
178+
(children, expandedChild) => {
179+
children[expandedChild.outlet] = expandedChild.segment;
180+
return children;
181+
},
182+
{} as {[outlet: string]: UrlSegmentGroup}),
183+
last(),
184+
);
149185
}
150186

151187
private expandSegment(
152188
ngModule: NgModuleRef<any>, segmentGroup: UrlSegmentGroup, routes: Route[],
153189
segments: UrlSegment[], outlet: string,
154190
allowRedirects: boolean): Observable<UrlSegmentGroup> {
155-
// We need to expand each outlet group independently to ensure that we not only load modules
156-
// for routes matching the given `outlet`, but also those which will be activated because
157-
// their path is empty string. This can result in multiple outlets being activated at once.
158-
const routesByOutlet: Map<string, Route[]> = groupRoutesByOutlet(routes);
159-
if (!routesByOutlet.has(outlet)) {
160-
routesByOutlet.set(outlet, []);
161-
}
162-
163-
const expandRoutes = (routes: Route[]) => {
164-
return from(routes).pipe(
165-
concatMap((r: Route) => {
166-
const expanded$ = this.expandSegmentAgainstRoute(
167-
ngModule, segmentGroup, routes, r, segments, outlet, allowRedirects);
168-
return expanded$.pipe(catchError(e => {
169-
if (e instanceof NoMatch) {
170-
return of(null);
171-
}
172-
throw e;
173-
}));
174-
}),
175-
first((s: UrlSegmentGroup|null): s is UrlSegmentGroup => s !== null),
176-
catchError(e => {
177-
if (e instanceof EmptyError || e.name === 'EmptyError') {
178-
if (noLeftoversInUrl(segmentGroup, segments, outlet)) {
179-
return of(new UrlSegmentGroup([], {}));
180-
}
181-
throw new NoMatch(segmentGroup);
191+
return from(routes).pipe(
192+
concatMap((r: any) => {
193+
const expanded$ = this.expandSegmentAgainstRoute(
194+
ngModule, segmentGroup, routes, r, segments, outlet, allowRedirects);
195+
return expanded$.pipe(catchError((e: any) => {
196+
if (e instanceof NoMatch) {
197+
return of(null);
182198
}
183199
throw e;
184-
}),
185-
);
186-
};
187-
188-
const expansions = Array.from(routesByOutlet.entries()).map(([routeOutlet, routes]) => {
189-
const expanded = expandRoutes(routes);
190-
// Map all results from outlets we aren't activating to `null` so they can be ignored later
191-
return routeOutlet === outlet ? expanded :
192-
expanded.pipe(map(() => null), catchError(() => of(null)));
193-
});
194-
return from(expansions)
195-
.pipe(
196-
combineAll(),
197-
first(),
198-
// Return only the expansion for the route outlet we are trying to activate.
199-
map(results => results.find(result => result !== null)!),
200-
);
200+
}));
201+
}),
202+
first((s): s is UrlSegmentGroup => !!s), catchError((e: any, _: any) => {
203+
if (e instanceof EmptyError || e.name === 'EmptyError') {
204+
if (noLeftoversInUrl(segmentGroup, segments, outlet)) {
205+
return of(new UrlSegmentGroup([], {}));
206+
}
207+
throw new NoMatch(segmentGroup);
208+
}
209+
throw e;
210+
}));
201211
}
202212

203213
private expandSegmentAgainstRoute(
204214
ngModule: NgModuleRef<any>, segmentGroup: UrlSegmentGroup, routes: Route[], route: Route,
205215
paths: UrlSegment[], outlet: string, allowRedirects: boolean): Observable<UrlSegmentGroup> {
206-
// Empty string segments are special because multiple outlets can match a single path, i.e.
207-
// `[{path: '', component: B}, {path: '', loadChildren: () => {}, outlet: "about"}]`
208-
if (getOutlet(route) !== outlet && route.path !== '') {
216+
if (!isImmediateMatch(route, segmentGroup, paths, outlet)) {
209217
return noMatch(segmentGroup);
210218
}
211219

212220
if (route.redirectTo === undefined) {
213-
return this.matchSegmentAgainstRoute(ngModule, segmentGroup, route, paths);
221+
return this.matchSegmentAgainstRoute(ngModule, segmentGroup, route, paths, outlet);
214222
}
215223

216224
if (allowRedirects && this.allowRedirects) {
@@ -269,7 +277,7 @@ class ApplyRedirects {
269277

270278
private matchSegmentAgainstRoute(
271279
ngModule: NgModuleRef<any>, rawSegmentGroup: UrlSegmentGroup, route: Route,
272-
segments: UrlSegment[]): Observable<UrlSegmentGroup> {
280+
segments: UrlSegment[], outlet: string): Observable<UrlSegmentGroup> {
273281
if (route.path === '**') {
274282
if (route.loadChildren) {
275283
return this.configLoader.load(ngModule.injector, route)
@@ -292,16 +300,11 @@ class ApplyRedirects {
292300
const childModule = routerConfig.module;
293301
const childConfig = routerConfig.routes;
294302

295-
const {segmentGroup, slicedSegments} =
303+
const {segmentGroup: splitSegmentGroup, slicedSegments} =
296304
split(rawSegmentGroup, consumedSegments, rawSlicedSegments, childConfig);
297-
// TODO(atscott): clearing the source segment and segment index shift is only necessary to
298-
// prevent failures in tests which assert exact object matches. The `split` is now shared
299-
// between applyRedirects and recognize and only the `recognize` step needs these properties.
300-
// Before the implementations were merged, the applyRedirects would not assign them.
301-
// We should be able to remove this logic as a "breaking change" but should do some more
302-
// investigation into the failures first.
303-
segmentGroup._sourceSegment = undefined;
304-
segmentGroup._segmentIndexShift = undefined;
305+
// See comment on the other call to `split` about why this is necessary.
306+
const segmentGroup =
307+
new UrlSegmentGroup(splitSegmentGroup.segments, splitSegmentGroup.children);
305308

306309
if (slicedSegments.length === 0 && segmentGroup.hasChildren()) {
307310
const expanded$ = this.expandChildren(childModule, childConfig, segmentGroup);
@@ -313,8 +316,10 @@ class ApplyRedirects {
313316
return of(new UrlSegmentGroup(consumedSegments, {}));
314317
}
315318

319+
const matchedOnOutlet = getOutlet(route) === outlet;
316320
const expanded$ = this.expandSegment(
317-
childModule, segmentGroup, childConfig, slicedSegments, PRIMARY_OUTLET, true);
321+
childModule, segmentGroup, childConfig, slicedSegments,
322+
matchedOnOutlet ? PRIMARY_OUTLET : outlet, true);
318323
return expanded$.pipe(
319324
map((cs: UrlSegmentGroup) =>
320325
new UrlSegmentGroup(consumedSegments.concat(cs.segments), cs.children)));
@@ -473,7 +478,6 @@ class ApplyRedirects {
473478
}
474479
}
475480

476-
477481
/**
478482
* When possible, merges the primary outlet child into the parent `UrlSegmentGroup`.
479483
*

0 commit comments

Comments
 (0)