Skip to content

Commit 3278966

Browse files
atscottdylhunn
atscott
authored and
dylhunn
committed
fix(router): Ensure newly resolved data is inherited by child routes (#52167)
The current way of computing a route's params and data recomputes inherited data from the inheritance root every time. When the inheritance strategy is "emptyOnly", this isn't necessarily the root of the tree, but some point along the way (it stops once it reaches an ancestor route with a component). Instead, this commit updates parameter inheritance to only inherit data directly from the parent route (again, instead of recomputing all inherited data back to the inheritance root). The only requirement for making this work is that the parent route data has already calculated and updated its own inherited data. This was really already a requirement -- parents need to be processed before children. In addition, the update to the inheritance algorithm in this commit requires more of an understanding that a resolver running higher up in the tree has to propagate inherited data downwards. The previous algorithm hid this knowledge because resolvers would recompute inherited data from the root when run. However, routes that did not have resolvers rerun or never had resolvers at all would not get the updated resolved data. fixes #51934 PR Close #52167
1 parent 8fff07c commit 3278966

File tree

5 files changed

+196
-153
lines changed

5 files changed

+196
-153
lines changed

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

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1262,6 +1262,9 @@
12621262
{
12631263
"name": "first"
12641264
},
1265+
{
1266+
"name": "flattenRouteTree"
1267+
},
12651268
{
12661269
"name": "flattenUnsubscriptionErrors"
12671270
},
@@ -1349,6 +1352,9 @@
13491352
{
13501353
"name": "getIdxOfMatchingSelector"
13511354
},
1355+
{
1356+
"name": "getInherited"
1357+
},
13521358
{
13531359
"name": "getInjectableDef"
13541360
},
@@ -1505,9 +1511,6 @@
15051511
{
15061512
"name": "incrementInitPhaseFlags"
15071513
},
1508-
{
1509-
"name": "inheritedParamsDataResolve"
1510-
},
15111514
{
15121515
"name": "initFeatures"
15131516
},
@@ -1886,6 +1889,9 @@
18861889
{
18871890
"name": "resetPreOrderHookFlags"
18881891
},
1892+
{
1893+
"name": "resolveData"
1894+
},
18891895
{
18901896
"name": "resolveForwardRef"
18911897
},

‎packages/router/src/operators/resolve_data.ts‎

Lines changed: 33 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -10,9 +10,9 @@ import {EnvironmentInjector, ProviderToken} from '@angular/core';
1010
import {EMPTY, from, MonoTypeOperatorFunction, Observable, of, throwError} from 'rxjs';
1111
import {catchError, concatMap, first, map, mapTo, mergeMap, takeLast, tap} from 'rxjs/operators';
1212

13-
import {ResolveData, Route} from '../models';
13+
import {ResolveData} from '../models';
1414
import {NavigationTransition} from '../navigation_transition';
15-
import {ActivatedRouteSnapshot, inheritedParamsDataResolve, RouterStateSnapshot} from '../router_state';
15+
import {ActivatedRouteSnapshot, getInherited, hasStaticTitle, RouterStateSnapshot} from '../router_state';
1616
import {RouteTitleKey} from '../shared';
1717
import {getDataKeys, wrapIntoObservable} from '../utils/collection';
1818
import {getClosestRouteInjector} from '../utils/config';
@@ -28,19 +28,42 @@ export function resolveData(
2828
if (!canActivateChecks.length) {
2929
return of(t);
3030
}
31-
let canActivateChecksResolved = 0;
32-
return from(canActivateChecks)
31+
const routesWithResolversToRun = canActivateChecks.map(check => check.route);
32+
const routesWithResolversSet = new Set(routesWithResolversToRun);
33+
const routesNeedingDataUpdates =
34+
// List all ActivatedRoutes in an array, starting from the parent of the first route to run
35+
// resolvers. We go from the parent because the first route might have siblings that also
36+
// run resolvers.
37+
flattenRouteTree(routesWithResolversToRun[0].parent!)
38+
// Remove the parent from the list -- we do not need to recompute its inherited data
39+
// because no resolvers at or above it run.
40+
.slice(1);
41+
let routesProcessed = 0;
42+
return from(routesNeedingDataUpdates)
3343
.pipe(
34-
concatMap(
35-
check =>
36-
runResolve(check.route, targetSnapshot!, paramsInheritanceStrategy, injector)),
37-
tap(() => canActivateChecksResolved++),
44+
concatMap(route => {
45+
if (routesWithResolversSet.has(route)) {
46+
return runResolve(route, targetSnapshot!, paramsInheritanceStrategy, injector);
47+
} else {
48+
route.data = getInherited(route, route.parent, paramsInheritanceStrategy).resolve;
49+
return of(void 0);
50+
}
51+
}),
52+
tap(() => routesProcessed++),
3853
takeLast(1),
39-
mergeMap(_ => canActivateChecksResolved === canActivateChecks.length ? of(t) : EMPTY),
54+
mergeMap(_ => routesProcessed === routesNeedingDataUpdates.length ? of(t) : EMPTY),
4055
);
4156
});
4257
}
4358

59+
/**
60+
* Returns the `ActivatedRouteSnapshot` tree as an array, using DFS to traverse the route tree.
61+
*/
62+
function flattenRouteTree(route: ActivatedRouteSnapshot): ActivatedRouteSnapshot[] {
63+
const descendants = route.children.map(child => flattenRouteTree(child)).flat();
64+
return [route, ...descendants];
65+
}
66+
4467
function runResolve(
4568
futureARS: ActivatedRouteSnapshot, futureRSS: RouterStateSnapshot,
4669
paramsInheritanceStrategy: 'emptyOnly'|'always', injector: EnvironmentInjector) {
@@ -51,10 +74,7 @@ function runResolve(
5174
}
5275
return resolveNode(resolve, futureARS, futureRSS, injector).pipe(map((resolvedData: any) => {
5376
futureARS._resolvedData = resolvedData;
54-
futureARS.data = inheritedParamsDataResolve(futureARS, paramsInheritanceStrategy).resolve;
55-
if (config && hasStaticTitle(config)) {
56-
futureARS.data[RouteTitleKey] = config.title;
57-
}
77+
futureARS.data = getInherited(futureARS, futureARS.parent, paramsInheritanceStrategy).resolve;
5878
return null;
5979
}));
6080
}
@@ -89,7 +109,3 @@ function getResolver(
89109
closestInjector.runInContext(() => resolver(futureARS, futureRSS));
90110
return wrapIntoObservable(resolverValue);
91111
}
92-
93-
function hasStaticTitle(config: Route) {
94-
return typeof config.title === 'string' || config.title === null;
95-
}

‎packages/router/src/recognize.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@ import {RuntimeErrorCode} from './errors';
1616
import {Data, LoadedRouterConfig, ResolveData, Route, Routes} from './models';
1717
import {runCanLoadGuards} from './operators/check_guards';
1818
import {RouterConfigLoader} from './router_config_loader';
19-
import {ActivatedRouteSnapshot, inheritedParamsDataResolve, ParamsInheritanceStrategy, RouterStateSnapshot} from './router_state';
19+
import {ActivatedRouteSnapshot, getInherited, ParamsInheritanceStrategy, RouterStateSnapshot} from './router_state';
2020
import {PRIMARY_OUTLET} from './shared';
2121
import {UrlSegment, UrlSegmentGroup, UrlSerializer, UrlTree} from './url_tree';
2222
import {last} from './utils/collection';
@@ -83,7 +83,7 @@ export class Recognizer {
8383
// We don't want to do this here so reassign them to the original.
8484
tree.queryParams = this.urlTree.queryParams;
8585
routeState.url = this.urlSerializer.serialize(tree);
86-
this.inheritParamsAndData(routeState._root);
86+
this.inheritParamsAndData(routeState._root, null);
8787
return {state: routeState, tree};
8888
}));
8989
}
@@ -105,14 +105,15 @@ export class Recognizer {
105105
}));
106106
}
107107

108-
inheritParamsAndData(routeNode: TreeNode<ActivatedRouteSnapshot>): void {
108+
inheritParamsAndData(
109+
routeNode: TreeNode<ActivatedRouteSnapshot>, parent: ActivatedRouteSnapshot|null): void {
109110
const route = routeNode.value;
111+
const i = getInherited(route, parent, this.paramsInheritanceStrategy);
110112

111-
const i = inheritedParamsDataResolve(route, this.paramsInheritanceStrategy);
112113
route.params = Object.freeze(i.params);
113114
route.data = Object.freeze(i.data);
114115

115-
routeNode.children.forEach(n => this.inheritParamsAndData(n));
116+
routeNode.children.forEach(n => this.inheritParamsAndData(n, route));
116117
}
117118

118119
processSegmentGroup(

‎packages/router/src/router_state.ts‎

Lines changed: 46 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {map} from 'rxjs/operators';
1212

1313
import {Data, ResolveData, Route} from './models';
1414
import {convertToParamMap, ParamMap, Params, PRIMARY_OUTLET, RouteTitleKey} from './shared';
15-
import {equalSegments, UrlSegment, UrlSegmentGroup, UrlTree} from './url_tree';
15+
import {equalSegments, UrlSegment, UrlTree} from './url_tree';
1616
import {shallowEqual, shallowEqualArrays} from './utils/collection';
1717
import {Tree, TreeNode} from './utils/tree';
1818

@@ -229,47 +229,52 @@ export type Inherited = {
229229

230230
/**
231231
* Returns the inherited params, data, and resolve for a given route.
232-
* By default, this only inherits values up to the nearest path-less or component-less route.
233-
* @internal
232+
*
233+
* By default, we do not inherit parent data unless the current route is path-less or the parent
234+
* route is component-less.
234235
*/
235-
export function inheritedParamsDataResolve(
236-
route: ActivatedRouteSnapshot,
236+
export function getInherited(
237+
route: ActivatedRouteSnapshot, parent: ActivatedRouteSnapshot|null,
237238
paramsInheritanceStrategy: ParamsInheritanceStrategy = 'emptyOnly'): Inherited {
238-
const pathFromRoot = route.pathFromRoot;
239-
240-
let inheritingStartingFrom = 0;
241-
if (paramsInheritanceStrategy !== 'always') {
242-
inheritingStartingFrom = pathFromRoot.length - 1;
243-
244-
while (inheritingStartingFrom >= 1) {
245-
const current = pathFromRoot[inheritingStartingFrom];
246-
const parent = pathFromRoot[inheritingStartingFrom - 1];
247-
// current route is an empty path => inherits its parent's params and data
248-
if (current.routeConfig && current.routeConfig.path === '') {
249-
inheritingStartingFrom--;
250-
251-
// parent is componentless => current route should inherit its params and data
252-
} else if (!parent.component && parent.routeConfig?.loadComponent === undefined) {
253-
inheritingStartingFrom--;
254-
255-
} else {
256-
break;
239+
let inherited: Inherited;
240+
const {routeConfig} = route;
241+
if (parent !== null &&
242+
(paramsInheritanceStrategy === 'always' ||
243+
// inherit parent data if route is empty path
244+
routeConfig?.path === '' ||
245+
// inherit parent data if parent was componentless
246+
(!parent.component && !parent.routeConfig?.loadComponent))) {
247+
inherited = {
248+
params: {...parent.params, ...route.params},
249+
data: {...parent.data, ...route.data},
250+
resolve: {
251+
// Snapshots are created with data inherited from parent and guards (i.e. canActivate) can
252+
// change data because it's not frozen...
253+
// This first line could be deleted chose to break/disallow mutating the `data` object in
254+
// guards.
255+
// Note that data from parents still override this mutated data so anyone relying on this
256+
// might be surprised that it doesn't work if parent data is inherited but otherwise does.
257+
...route.data,
258+
// Ensure inherited resolved data overrides inherited static data
259+
...parent.data,
260+
// static data from the current route overrides any inherited data
261+
...routeConfig?.data,
262+
// resolved data from current route overrides everything
263+
...route._resolvedData,
257264
}
258-
}
265+
};
266+
} else {
267+
inherited = {
268+
params: route.params,
269+
data: route.data,
270+
resolve: {...route.data, ...(route._resolvedData ?? {})}
271+
};
259272
}
260273

261-
return flattenInherited(pathFromRoot.slice(inheritingStartingFrom));
262-
}
263-
264-
/** @internal */
265-
function flattenInherited(pathFromRoot: ActivatedRouteSnapshot[]): Inherited {
266-
return pathFromRoot.reduce((res, curr) => {
267-
const params = {...res.params, ...curr.params};
268-
const data = {...res.data, ...curr.data};
269-
const resolve =
270-
{...curr.data, ...res.resolve, ...curr.routeConfig?.data, ...curr._resolvedData};
271-
return {params, data, resolve};
272-
}, {params: {}, data: {}, resolve: {}});
274+
if (routeConfig && hasStaticTitle(routeConfig)) {
275+
inherited.resolve[RouteTitleKey] = routeConfig.title;
276+
}
277+
return inherited;
273278
}
274279

275280
/**
@@ -493,3 +498,7 @@ export function equalParamsAndUrlSegments(
493498
return equalUrlParams && !parentsMismatch &&
494499
(!a.parent || equalParamsAndUrlSegments(a.parent, b.parent!));
495500
}
501+
502+
export function hasStaticTitle(config: Route) {
503+
return typeof config.title === 'string' || config.title === null;
504+
}

0 commit comments

Comments
 (0)