fix(router): Ensure newly resolved data is inherited by child routes (a…

…ngular#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 angular#51934

PR Close angular#52167

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"
   },

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;
-}

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<ActivatedRouteSnapshot>): void {
+  inheritParamsAndData(
+      routeNode: TreeNode<ActivatedRouteSnapshot>, 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(

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;
+}