From 327896606832bf6fbfc8f23989755123028136a8 Mon Sep 17 00:00:00 2001 From: Andrew Scott Date: Wed, 11 Oct 2023 09:09:08 -0700 Subject: [PATCH] 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 --- .../router/bundle.golden_symbols.json | 12 +- packages/router/src/operators/resolve_data.ts | 50 +++-- packages/router/src/recognize.ts | 11 +- packages/router/src/router_state.ts | 83 ++++---- .../test/operators/resolve_data.spec.ts | 193 +++++++++--------- 5 files changed, 196 insertions(+), 153 deletions(-) diff --git a/packages/core/test/bundling/router/bundle.golden_symbols.json b/packages/core/test/bundling/router/bundle.golden_symbols.json index 983c02df440fc..d127c2f41f206 100644 --- a/packages/core/test/bundling/router/bundle.golden_symbols.json +++ b/packages/core/test/bundling/router/bundle.golden_symbols.json @@ -1262,6 +1262,9 @@ { "name": "first" }, + { + "name": "flattenRouteTree" + }, { "name": "flattenUnsubscriptionErrors" }, @@ -1349,6 +1352,9 @@ { "name": "getIdxOfMatchingSelector" }, + { + "name": "getInherited" + }, { "name": "getInjectableDef" }, @@ -1505,9 +1511,6 @@ { "name": "incrementInitPhaseFlags" }, - { - "name": "inheritedParamsDataResolve" - }, { "name": "initFeatures" }, @@ -1886,6 +1889,9 @@ { "name": "resetPreOrderHookFlags" }, + { + "name": "resolveData" + }, { "name": "resolveForwardRef" }, diff --git a/packages/router/src/operators/resolve_data.ts b/packages/router/src/operators/resolve_data.ts index fefc7729f82a6..38ca2c5484267 100644 --- a/packages/router/src/operators/resolve_data.ts +++ b/packages/router/src/operators/resolve_data.ts @@ -10,9 +10,9 @@ import {EnvironmentInjector, ProviderToken} from '@angular/core'; import {EMPTY, from, MonoTypeOperatorFunction, Observable, of, throwError} from 'rxjs'; import {catchError, concatMap, first, map, mapTo, mergeMap, takeLast, tap} from 'rxjs/operators'; -import {ResolveData, Route} from '../models'; +import {ResolveData} from '../models'; import {NavigationTransition} from '../navigation_transition'; -import {ActivatedRouteSnapshot, inheritedParamsDataResolve, RouterStateSnapshot} from '../router_state'; +import {ActivatedRouteSnapshot, getInherited, hasStaticTitle, RouterStateSnapshot} from '../router_state'; import {RouteTitleKey} from '../shared'; import {getDataKeys, wrapIntoObservable} from '../utils/collection'; import {getClosestRouteInjector} from '../utils/config'; @@ -28,19 +28,42 @@ export function resolveData( if (!canActivateChecks.length) { return of(t); } - let canActivateChecksResolved = 0; - return from(canActivateChecks) + const routesWithResolversToRun = canActivateChecks.map(check => check.route); + const routesWithResolversSet = new Set(routesWithResolversToRun); + const routesNeedingDataUpdates = + // List all ActivatedRoutes in an array, starting from the parent of the first route to run + // resolvers. We go from the parent because the first route might have siblings that also + // run resolvers. + flattenRouteTree(routesWithResolversToRun[0].parent!) + // Remove the parent from the list -- we do not need to recompute its inherited data + // because no resolvers at or above it run. + .slice(1); + let routesProcessed = 0; + return from(routesNeedingDataUpdates) .pipe( - concatMap( - check => - runResolve(check.route, targetSnapshot!, paramsInheritanceStrategy, injector)), - tap(() => canActivateChecksResolved++), + concatMap(route => { + if (routesWithResolversSet.has(route)) { + return runResolve(route, targetSnapshot!, paramsInheritanceStrategy, injector); + } else { + route.data = getInherited(route, route.parent, paramsInheritanceStrategy).resolve; + return of(void 0); + } + }), + tap(() => routesProcessed++), takeLast(1), - mergeMap(_ => canActivateChecksResolved === canActivateChecks.length ? of(t) : EMPTY), + mergeMap(_ => routesProcessed === routesNeedingDataUpdates.length ? of(t) : EMPTY), ); }); } +/** + * Returns the `ActivatedRouteSnapshot` tree as an array, using DFS to traverse the route tree. + */ +function flattenRouteTree(route: ActivatedRouteSnapshot): ActivatedRouteSnapshot[] { + const descendants = route.children.map(child => flattenRouteTree(child)).flat(); + return [route, ...descendants]; +} + function runResolve( futureARS: ActivatedRouteSnapshot, futureRSS: RouterStateSnapshot, paramsInheritanceStrategy: 'emptyOnly'|'always', injector: EnvironmentInjector) { @@ -51,10 +74,7 @@ function runResolve( } return resolveNode(resolve, futureARS, futureRSS, injector).pipe(map((resolvedData: any) => { futureARS._resolvedData = resolvedData; - futureARS.data = inheritedParamsDataResolve(futureARS, paramsInheritanceStrategy).resolve; - if (config && hasStaticTitle(config)) { - futureARS.data[RouteTitleKey] = config.title; - } + futureARS.data = getInherited(futureARS, futureARS.parent, paramsInheritanceStrategy).resolve; return null; })); } @@ -89,7 +109,3 @@ function getResolver( closestInjector.runInContext(() => resolver(futureARS, futureRSS)); return wrapIntoObservable(resolverValue); } - -function hasStaticTitle(config: Route) { - return typeof config.title === 'string' || config.title === null; -} diff --git a/packages/router/src/recognize.ts b/packages/router/src/recognize.ts index 50eeeeef51757..0a8bdfc87ae8f 100644 --- a/packages/router/src/recognize.ts +++ b/packages/router/src/recognize.ts @@ -16,7 +16,7 @@ import {RuntimeErrorCode} from './errors'; import {Data, LoadedRouterConfig, ResolveData, Route, Routes} from './models'; import {runCanLoadGuards} from './operators/check_guards'; import {RouterConfigLoader} from './router_config_loader'; -import {ActivatedRouteSnapshot, inheritedParamsDataResolve, ParamsInheritanceStrategy, RouterStateSnapshot} from './router_state'; +import {ActivatedRouteSnapshot, getInherited, ParamsInheritanceStrategy, RouterStateSnapshot} from './router_state'; import {PRIMARY_OUTLET} from './shared'; import {UrlSegment, UrlSegmentGroup, UrlSerializer, UrlTree} from './url_tree'; import {last} from './utils/collection'; @@ -83,7 +83,7 @@ export class Recognizer { // We don't want to do this here so reassign them to the original. tree.queryParams = this.urlTree.queryParams; routeState.url = this.urlSerializer.serialize(tree); - this.inheritParamsAndData(routeState._root); + this.inheritParamsAndData(routeState._root, null); return {state: routeState, tree}; })); } @@ -105,14 +105,15 @@ export class Recognizer { })); } - inheritParamsAndData(routeNode: TreeNode): void { + inheritParamsAndData( + routeNode: TreeNode, parent: ActivatedRouteSnapshot|null): void { const route = routeNode.value; + const i = getInherited(route, parent, this.paramsInheritanceStrategy); - const i = inheritedParamsDataResolve(route, this.paramsInheritanceStrategy); route.params = Object.freeze(i.params); route.data = Object.freeze(i.data); - routeNode.children.forEach(n => this.inheritParamsAndData(n)); + routeNode.children.forEach(n => this.inheritParamsAndData(n, route)); } processSegmentGroup( diff --git a/packages/router/src/router_state.ts b/packages/router/src/router_state.ts index 06b4cc9ff56aa..a8ed74a2605e3 100644 --- a/packages/router/src/router_state.ts +++ b/packages/router/src/router_state.ts @@ -12,7 +12,7 @@ import {map} from 'rxjs/operators'; import {Data, ResolveData, Route} from './models'; import {convertToParamMap, ParamMap, Params, PRIMARY_OUTLET, RouteTitleKey} from './shared'; -import {equalSegments, UrlSegment, UrlSegmentGroup, UrlTree} from './url_tree'; +import {equalSegments, UrlSegment, UrlTree} from './url_tree'; import {shallowEqual, shallowEqualArrays} from './utils/collection'; import {Tree, TreeNode} from './utils/tree'; @@ -229,47 +229,52 @@ export type Inherited = { /** * Returns the inherited params, data, and resolve for a given route. - * By default, this only inherits values up to the nearest path-less or component-less route. - * @internal + * + * By default, we do not inherit parent data unless the current route is path-less or the parent + * route is component-less. */ -export function inheritedParamsDataResolve( - route: ActivatedRouteSnapshot, +export function getInherited( + route: ActivatedRouteSnapshot, parent: ActivatedRouteSnapshot|null, paramsInheritanceStrategy: ParamsInheritanceStrategy = 'emptyOnly'): Inherited { - const pathFromRoot = route.pathFromRoot; - - let inheritingStartingFrom = 0; - if (paramsInheritanceStrategy !== 'always') { - inheritingStartingFrom = pathFromRoot.length - 1; - - while (inheritingStartingFrom >= 1) { - const current = pathFromRoot[inheritingStartingFrom]; - const parent = pathFromRoot[inheritingStartingFrom - 1]; - // current route is an empty path => inherits its parent's params and data - if (current.routeConfig && current.routeConfig.path === '') { - inheritingStartingFrom--; - - // parent is componentless => current route should inherit its params and data - } else if (!parent.component && parent.routeConfig?.loadComponent === undefined) { - inheritingStartingFrom--; - - } else { - break; + let inherited: Inherited; + const {routeConfig} = route; + if (parent !== null && + (paramsInheritanceStrategy === 'always' || + // inherit parent data if route is empty path + routeConfig?.path === '' || + // inherit parent data if parent was componentless + (!parent.component && !parent.routeConfig?.loadComponent))) { + inherited = { + params: {...parent.params, ...route.params}, + data: {...parent.data, ...route.data}, + resolve: { + // Snapshots are created with data inherited from parent and guards (i.e. canActivate) can + // change data because it's not frozen... + // This first line could be deleted chose to break/disallow mutating the `data` object in + // guards. + // Note that data from parents still override this mutated data so anyone relying on this + // might be surprised that it doesn't work if parent data is inherited but otherwise does. + ...route.data, + // Ensure inherited resolved data overrides inherited static data + ...parent.data, + // static data from the current route overrides any inherited data + ...routeConfig?.data, + // resolved data from current route overrides everything + ...route._resolvedData, } - } + }; + } else { + inherited = { + params: route.params, + data: route.data, + resolve: {...route.data, ...(route._resolvedData ?? {})} + }; } - return flattenInherited(pathFromRoot.slice(inheritingStartingFrom)); -} - -/** @internal */ -function flattenInherited(pathFromRoot: ActivatedRouteSnapshot[]): Inherited { - return pathFromRoot.reduce((res, curr) => { - const params = {...res.params, ...curr.params}; - const data = {...res.data, ...curr.data}; - const resolve = - {...curr.data, ...res.resolve, ...curr.routeConfig?.data, ...curr._resolvedData}; - return {params, data, resolve}; - }, {params: {}, data: {}, resolve: {}}); + if (routeConfig && hasStaticTitle(routeConfig)) { + inherited.resolve[RouteTitleKey] = routeConfig.title; + } + return inherited; } /** @@ -493,3 +498,7 @@ export function equalParamsAndUrlSegments( return equalUrlParams && !parentsMismatch && (!a.parent || equalParamsAndUrlSegments(a.parent, b.parent!)); } + +export function hasStaticTitle(config: Route) { + return typeof config.title === 'string' || config.title === null; +} diff --git a/packages/router/test/operators/resolve_data.spec.ts b/packages/router/test/operators/resolve_data.spec.ts index a87a587291711..98e0564f1bf7b 100644 --- a/packages/router/test/operators/resolve_data.spec.ts +++ b/packages/router/test/operators/resolve_data.spec.ts @@ -6,122 +6,133 @@ * found in the LICENSE file at https://angular.io/license */ -import {EnvironmentInjector, Injector} from '@angular/core'; +import {Component} from '@angular/core'; import {TestBed} from '@angular/core/testing'; +import {provideRouter, Router} from '@angular/router'; +import {RouterTestingHarness} from '@angular/router/testing'; import {EMPTY, interval, NEVER, of} from 'rxjs'; -import {TestScheduler} from 'rxjs/testing'; - -import {resolveData} from '../../src/operators/resolve_data'; describe('resolveData operator', () => { - let testScheduler: TestScheduler; - let injector: EnvironmentInjector; - - beforeEach(() => { + it('should take only the first emitted value of every resolver', async () => { TestBed.configureTestingModule({ - providers: [ - {provide: 'resolveTwo', useValue: (a: any, b: any) => of(2)}, - {provide: 'resolveFour', useValue: (a: any, b: any) => 4}, - {provide: 'resolveEmpty', useValue: (a: any, b: any) => EMPTY}, - {provide: 'resolveInterval', useValue: (a: any, b: any) => interval()}, - {provide: 'resolveNever', useValue: (a: any, b: any) => NEVER}, - ] + providers: [provideRouter([{path: '**', children: [], resolve: {e1: () => interval()}}])] }); - }); - beforeEach(() => { - testScheduler = new TestScheduler(assertDeepEquals); - }); - beforeEach(() => { - injector = TestBed.inject(EnvironmentInjector); + await RouterTestingHarness.create('/'); + expect(TestBed.inject(Router).routerState.root.firstChild?.snapshot.data).toEqual({e1: 0}); }); - it('should re-emit updated value from source after all resolvers emit and complete', () => { - testScheduler.run(({hot, cold, expectObservable}) => { - const transition: any = createTransition({e1: 'resolveTwo'}, {e2: 'resolveFour'}); - const source = cold('-(t|)', {t: deepClone(transition)}); - const expected = '-(t|)'; - const outputTransition = deepClone(transition); - outputTransition.guards.canActivateChecks[0].route._resolvedData = {e1: 2}; - outputTransition.guards.canActivateChecks[1].route._resolvedData = {e2: 4}; + it('should cancel navigation if a resolver does not complete', async () => { + TestBed.configureTestingModule( + {providers: [provideRouter([{path: '**', children: [], resolve: {e1: () => EMPTY}}])]}); + await RouterTestingHarness.create('/a'); + expect(TestBed.inject(Router).url).toEqual('/'); + }); - expectObservable(source.pipe(resolveData('emptyOnly', injector))).toBe(expected, { - t: outputTransition - }); + it('should cancel navigation if 1 of 2 resolvers does not emit', async () => { + TestBed.configureTestingModule({ + providers: + [provideRouter([{path: '**', children: [], resolve: {e0: () => of(0), e1: () => EMPTY}}])] }); + await RouterTestingHarness.create('/a'); + expect(TestBed.inject(Router).url).toEqual('/'); }); - it('should take only the first emitted value of every resolver', () => { - testScheduler.run(({cold, expectObservable}) => { - const transition: any = createTransition({e1: 'resolveInterval'}); - const source = cold('-(t|)', {t: deepClone(transition)}); - const expected = '-(t|)'; - const outputTransition = deepClone(transition); - outputTransition.guards.canActivateChecks[0].route._resolvedData = {e1: 0}; - - expectObservable(source.pipe(resolveData('emptyOnly', injector))).toBe(expected, { - t: outputTransition - }); + it('should complete instantly if at least one resolver doesn\'t emit', async () => { + TestBed.configureTestingModule({ + providers: + [provideRouter([{path: '**', children: [], resolve: {e0: () => EMPTY, e1: () => NEVER}}])] }); + await RouterTestingHarness.create('/a'); + expect(TestBed.inject(Router).url).toEqual('/'); }); - it('should re-emit value from source when there are no resolvers', () => { - testScheduler.run(({hot, cold, expectObservable}) => { - const transition: any = createTransition({}); - const source = cold('-(t|)', {t: deepClone(transition)}); - const expected = '-(t|)'; - const outputTransition = deepClone(transition); - outputTransition.guards.canActivateChecks[0].route._resolvedData = {}; + it('should update children inherited data when resolvers run', async () => { + let value = 0; + TestBed.configureTestingModule({ + providers: [provideRouter([{ + path: 'a', + children: [{path: 'b', children: []}], + resolve: {d0: () => ++value}, + runGuardsAndResolvers: 'always', + }])] + }); + const harness = await RouterTestingHarness.create('/a/b'); + expect(TestBed.inject(Router).routerState.root.firstChild?.snapshot.data).toEqual({d0: 1}); + expect(TestBed.inject(Router).routerState.root.firstChild?.firstChild?.snapshot.data).toEqual({ + d0: 1 + }); - expectObservable(source.pipe(resolveData('emptyOnly', injector))).toBe(expected, { - t: outputTransition - }); + await harness.navigateByUrl('/a/b#new'); + expect(TestBed.inject(Router).routerState.root.firstChild?.snapshot.data).toEqual({d0: 2}); + expect(TestBed.inject(Router).routerState.root.firstChild?.firstChild?.snapshot.data).toEqual({ + d0: 2 }); }); - it('should not emit when there\'s one resolver that doesn\'t emit', () => { - testScheduler.run(({hot, cold, expectObservable}) => { - const transition: any = createTransition({e2: 'resolveEmpty'}); - const source = cold('-(t|)', {t: deepClone(transition)}); - const expected = '-|'; - expectObservable(source.pipe(resolveData('emptyOnly', injector))).toBe(expected); + it('should have correct data when parent resolver runs but data is not inherited', async () => { + @Component({template: ''}) + class Empty { + } + + TestBed.configureTestingModule({ + providers: [provideRouter([{ + path: 'a', + component: Empty, + data: {parent: 'parent'}, + resolve: {other: () => 'other'}, + children: [{ + path: 'b', + data: {child: 'child'}, + component: Empty, + }] + }])] }); + await RouterTestingHarness.create('/a/b'); + const rootSnapshot = TestBed.inject(Router).routerState.root.firstChild!.snapshot; + expect(rootSnapshot.data).toEqual({parent: 'parent', other: 'other'}); + expect(rootSnapshot.firstChild!.data).toEqual({child: 'child'}); }); - it('should not emit if at least one resolver doesn\'t emit', () => { - testScheduler.run(({hot, cold, expectObservable}) => { - const transition: any = createTransition({e1: 'resolveTwo'}, {e2: 'resolveEmpty'}); - const source = cold('-(t|)', {t: deepClone(transition)}); - const expected = '-|'; - expectObservable(source.pipe(resolveData('emptyOnly', injector))).toBe(expected); + it('should have static title when there is a resolver', async () => { + @Component({template: ''}) + class Empty { + } + + TestBed.configureTestingModule({ + providers: [provideRouter([{ + path: 'a', + title: 'a title', + component: Empty, + resolve: {other: () => 'other'}, + children: [{ + path: 'b', + title: 'b title', + component: Empty, + resolve: {otherb: () => 'other b'}, + }] + }])] }); + await RouterTestingHarness.create('/a/b'); + const rootSnapshot = TestBed.inject(Router).routerState.root.firstChild!.snapshot; + expect(rootSnapshot.title).toBe('a title'); + expect(rootSnapshot.firstChild!.title).toBe('b title'); }); - it('should complete instantly if at least one resolver doesn\'t emit', () => { - testScheduler.run(({cold, expectObservable}) => { - const transition: any = createTransition({e1: 'resolveEmpty', e2: 'resolveNever'}); - const source = cold('-(t|)', {t: deepClone(transition)}); - const expected = '-|'; - expectObservable(source.pipe(resolveData('emptyOnly', injector))).toBe(expected); + it('should inherit resolved data from parent of parent route', async () => { + @Component({template: ''}) + class Empty { + } + + TestBed.configureTestingModule({ + providers: [provideRouter([{ + path: 'a', + resolve: {aResolve: () => 'a'}, + children: + [{path: 'b', resolve: {bResolve: () => 'b'}, children: [{path: 'c', component: Empty}]}] + }])] }); + await RouterTestingHarness.create('/a/b/c'); + const rootSnapshot = TestBed.inject(Router).routerState.root.firstChild!.snapshot; + expect(rootSnapshot.firstChild!.firstChild!.data).toEqual({bResolve: 'b', aResolve: 'a'}); }); }); - -function assertDeepEquals(a: any, b: any) { - return expect(a).toEqual(b); -} - -function createTransition(...resolvers: {[key: string]: string}[]) { - return { - targetSnapshot: {}, - guards: { - canActivateChecks: - resolvers.map(resolver => ({ - route: {_resolve: resolver, pathFromRoot: [{url: '/'}], data: {}}, - })), - }, - }; -} - -function deepClone(obj: T): T { - return JSON.parse(JSON.stringify(obj)) as T; -}