fix(router): reuse route strategy fix (#43791)

Currently, it's impossible to cache (detach/attach) parent route without caching child routes.
This produces a bug, when navigating from a/b to c, then to a/d, where a route is cached.
On the last navigation, we incorrectly restore a/b route instead of a/d.
This change introduces new behavior: if the route should be detached/attached,
we do so, but we check also child routes recursively.

Fixes #17333

PR Close #43791

goldens/size-tracking/integration-payloads.json

@@ -42,7 +42,7 @@
     "master": {
       "uncompressed": {
         "runtime": 2843,
-        "main": 238453,
+        "main": 237895,
         "polyfills": 37064,
         "src_app_lazy_lazy_module_ts": 795
       }

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

@@ -941,9 +941,6 @@
   {
     "name": "advanceActivatedRoute"
   },
-  {
-    "name": "advanceActivatedRouteNodeAndItsChildren"
-  },
   {
     "name": "allocExpando"
   },
@@ -1922,9 +1919,6 @@
   {
     "name": "setDirectiveInputsWhichShadowsStyling"
   },
-  {
-    "name": "setFutureSnapshotsOfActivatedRoutes"
-  },
   {
     "name": "setIncludeViewProviders"
   },

packages/router/src/create_router_state.ts

@@ -34,7 +34,8 @@ function createNode(
       const detachedRouteHandle = routeReuseStrategy.retrieve(curr.value);
       if (detachedRouteHandle !== null) {
         const tree = (detachedRouteHandle as DetachedRouteHandleInternal).route;
-        setFutureSnapshotsOfActivatedRoutes(curr, tree);
+        tree.value._futureSnapshot = curr.value;
+        tree.children = curr.children.map(c => createNode(routeReuseStrategy, c));
         return tree;
       }
     }
@@ -45,20 +46,6 @@ function createNode(
   }
 }
 
-function setFutureSnapshotsOfActivatedRoutes(
-    curr: TreeNode<ActivatedRouteSnapshot>, result: TreeNode<ActivatedRoute>): void {
-  if (curr.value.routeConfig !== result.value.routeConfig) {
-    throw new Error('Cannot reattach ActivatedRouteSnapshot created from a different route');
-  }
-  if (curr.children.length !== result.children.length) {
-    throw new Error('Cannot reattach ActivatedRouteSnapshot with a different number of children');
-  }
-  result.value._futureSnapshot = curr.value;
-  for (let i = 0; i < curr.children.length; ++i) {
-    setFutureSnapshotsOfActivatedRoutes(curr.children[i], result.children[i]);
-  }
-}
-
 function createOrReuseChildren(
     routeReuseStrategy: RouteReuseStrategy, curr: TreeNode<ActivatedRouteSnapshot>,
     prevState: TreeNode<ActivatedRoute>) {

packages/router/src/operators/activate_routes.ts

@@ -99,6 +99,13 @@ export class ActivateRoutes {
   private detachAndStoreRouteSubtree(
       route: TreeNode<ActivatedRoute>, parentContexts: ChildrenOutletContexts): void {
     const context = parentContexts.getContext(route.value.outlet);
+    const contexts = context && route.value.component ? context.children : parentContexts;
+    const children: {[outletName: string]: TreeNode<ActivatedRoute>} = nodeChildrenAsMap(route);
+
+    for (const childOutlet of Object.keys(children)) {
+      this.deactivateRouteAndItsChildren(children[childOutlet], contexts);
+    }
+
     if (context && context.outlet) {
       const componentRef = context.outlet.detach();
       const contexts = context.children.onOutletDeactivated();
@@ -179,7 +186,9 @@ export class ActivateRoutes {
             // Otherwise attach from `RouterOutlet.ngOnInit` when it is instantiated
             context.outlet.attach(stored.componentRef, stored.route.value);
           }
-          advanceActivatedRouteNodeAndItsChildren(stored.route);
+
+          advanceActivatedRoute(stored.route.value);
+          this.activateChildRoutes(futureNode, null, context.children);
         } else {
           const config = parentLoadedConfig(future.snapshot);
           const cmpFactoryResolver = config ? config.module.componentFactoryResolver : null;
@@ -203,11 +212,6 @@ export class ActivateRoutes {
   }
 }
 
-function advanceActivatedRouteNodeAndItsChildren(node: TreeNode<ActivatedRoute>): void {
-  advanceActivatedRoute(node.value);
-  node.children.forEach(advanceActivatedRouteNodeAndItsChildren);
-}
-
 function parentLoadedConfig(snapshot: ActivatedRouteSnapshot): LoadedRouterConfig|null {
   for (let s = snapshot.parent; s; s = s.parent) {
     const route = s.routeConfig;

packages/router/test/integration.spec.ts

@@ -8,7 +8,7 @@
 
 import {APP_BASE_HREF, CommonModule, HashLocationStrategy, Location, LOCATION_INITIALIZED, LocationStrategy, PlatformLocation} from '@angular/common';
 import {SpyLocation} from '@angular/common/testing';
-import {ChangeDetectionStrategy, Component, EventEmitter, Injectable, NgModule, NgModuleRef, NgZone, OnDestroy, ViewChild, ɵConsole as Console, ɵNoopNgZone as NoopNgZone} from '@angular/core';
+import {ChangeDetectionStrategy, Component, EventEmitter, Inject, Injectable, InjectionToken, NgModule, NgModuleRef, NgZone, OnDestroy, ViewChild, ɵConsole as Console, ɵNoopNgZone as NoopNgZone} from '@angular/core';
 import {ComponentFixture, fakeAsync, inject, TestBed, tick} from '@angular/core/testing';
 import {By} from '@angular/platform-browser/src/dom/debug/by';
 import {expect} from '@angular/platform-browser/testing/src/matchers';
@@ -5711,9 +5711,11 @@ describe('Integration', () => {
   describe('Custom Route Reuse Strategy', () => {
     class AttachDetachReuseStrategy implements RouteReuseStrategy {
       stored: {[k: string]: DetachedRouteHandle} = {};
+      pathsToDetach = ['a'];
 
       shouldDetach(route: ActivatedRouteSnapshot): boolean {
-        return route.routeConfig!.path === 'a';
+        return typeof route.routeConfig!.path !== 'undefined' &&
+            this.pathsToDetach.includes(route.routeConfig!.path);
       }
 
       store(route: ActivatedRouteSnapshot, detachedTree: DetachedRouteHandle): void {
@@ -5827,6 +5829,7 @@ describe('Integration', () => {
          const fixture = createRoot(router, RootCmp);
 
          router.routeReuseStrategy = new AttachDetachReuseStrategy();
+         (router.routeReuseStrategy as AttachDetachReuseStrategy).pathsToDetach = ['a', 'b'];
          spyOn(router.routeReuseStrategy, 'retrieve').and.callThrough();
 
          router.resetConfig([
@@ -5857,7 +5860,7 @@ describe('Integration', () => {
          router.navigateByUrl('/a;p=1/b;p=2');
          advance(fixture);
          // We retrieve both the stored route snapshots
-         expect(router.routeReuseStrategy.retrieve).toHaveBeenCalledTimes(2);
+         expect(router.routeReuseStrategy.retrieve).toHaveBeenCalledTimes(4);
          const teamCmp2 = fixture.debugElement.children[1].componentInstance;
          const simpleCmp2 = fixture.debugElement.children[1].children[1].componentInstance;
          expect(location.path()).toEqual('/a;p=1/b;p=2');
@@ -5868,6 +5871,7 @@ describe('Integration', () => {
          expect(teamCmp.route.snapshot).toBe(router.routerState.snapshot.root.firstChild);
          expect(teamCmp.route.snapshot.params).toEqual({p: '1'});
          expect(teamCmp.route.firstChild.snapshot.params).toEqual({p: '2'});
+         expect(teamCmp.recordedParams).toEqual([{}, {p: '1'}]);
        })));
 
     it('should support shorter lifecycles',
@@ -6019,6 +6023,66 @@ describe('Integration', () => {
          advance(fixture);
          expect(fixture.debugElement.query(By.directive(SimpleCmp))).toBeTruthy();
        }));
+
+    it('should allow to attach parent route with fresh child route', fakeAsync(() => {
+         const CREATED_COMPS = new InjectionToken<string[]>('CREATED_COMPS');
+
+         @Component({selector: 'root', template: `<router-outlet></router-outlet>`})
+         class Root {
+         }
+
+         @Component({selector: 'parent', template: `<router-outlet></router-outlet>`})
+         class Parent {
+           constructor(@Inject(CREATED_COMPS) createdComps: string[]) {
+             createdComps.push('parent');
+           }
+         }
+
+         @Component({selector: 'child', template: `child`})
+         class Child {
+           constructor(@Inject(CREATED_COMPS) createdComps: string[]) {
+             createdComps.push('child');
+           }
+         }
+
+         @NgModule({
+           declarations: [Root, Parent, Child],
+           imports: [
+             CommonModule,
+             RouterTestingModule.withRoutes([
+               {path: 'a', component: Parent, children: [{path: 'b', component: Child}]},
+               {path: 'c', component: SimpleCmp}
+             ]),
+           ],
+           providers: [
+             {provide: RouteReuseStrategy, useClass: AttachDetachReuseStrategy},
+             {provide: CREATED_COMPS, useValue: []}
+           ]
+         })
+         class TestModule {
+         }
+         TestBed.configureTestingModule({imports: [TestModule]});
+
+         const router = TestBed.inject(Router);
+         const fixture = createRoot(router, Root);
+         const createdComps = TestBed.inject(CREATED_COMPS);
+
+         expect(createdComps).toEqual([]);
+
+         router.navigateByUrl('/a/b');
+         advance(fixture);
+         expect(createdComps).toEqual(['parent', 'child']);
+
+         router.navigateByUrl('/c');
+         advance(fixture);
+         expect(createdComps).toEqual(['parent', 'child']);
+
+         // 'a' parent route will be reused by the `RouteReuseStrategy`, child 'b' should be
+         // recreated
+         router.navigateByUrl('/a/b');
+         advance(fixture);
+         expect(createdComps).toEqual(['parent', 'child', 'child']);
+       }));
   });
 });