fix(router): Only retrieve stored route when reuse strategy indicates it should reattach (#30263)

When creating the router state, the `RouteReuseStrategy#retrieve` should
only be called when `RouteReuseStrategy#shouldAttach` returns `true`.
That is, we should only retrieve a stored route when the reuse strategy
indicates that there is one stored and that it should be reattached.

This now matches the behavior in the route activation:
1d12c50f63/packages/router/src/operators/activate_routes.ts (L170-L172)

Fixes #23162

PR Close #30263

packages/router/src/create_router_state.ts

@@ -28,21 +28,20 @@ function createNode(
     value._futureSnapshot = curr.value;
     const children = createOrReuseChildren(routeReuseStrategy, curr, prevState);
     return new TreeNode<ActivatedRoute>(value, children);
-
-    // retrieve an activated route that is used to be displayed, but is not currently displayed
   } else {
-    const detachedRouteHandle =
-        <DetachedRouteHandleInternal>routeReuseStrategy.retrieve(curr.value);
-    if (detachedRouteHandle) {
-      const tree: TreeNode<ActivatedRoute> = detachedRouteHandle.route;
-      setFutureSnapshotsOfActivatedRoutes(curr, tree);
-      return tree;
-
-    } else {
-      const value = createActivatedRoute(curr.value);
-      const children = curr.children.map(c => createNode(routeReuseStrategy, c));
-      return new TreeNode<ActivatedRoute>(value, children);
+    if (routeReuseStrategy.shouldAttach(curr.value)) {
+      // retrieve an activated route that is used to be displayed, but is not currently displayed
+      const detachedRouteHandle = routeReuseStrategy.retrieve(curr.value);
+      if (detachedRouteHandle !== null) {
+        const tree = (detachedRouteHandle as DetachedRouteHandleInternal).route;
+        setFutureSnapshotsOfActivatedRoutes(curr, tree);
+        return tree;
+      }
     }
+
+    const value = createActivatedRoute(curr.value);
+    const children = curr.children.map(c => createNode(routeReuseStrategy, c));
+    return new TreeNode<ActivatedRoute>(value, children);
   }
 }
 

packages/router/test/create_router_state.spec.ts

@@ -92,25 +92,18 @@ describe('create router state', () => {
     checkActivatedRoute(currC[1], ComponentB, 'right');
   });
 
-  it('should cache the retrieved routeReuseStrategy', () => {
+  it('should not retrieve routes when `shouldAttach` is always false', () => {
     const config = [
       {path: 'a', component: ComponentA}, {path: 'b', component: ComponentB, outlet: 'left'},
       {path: 'c', component: ComponentC, outlet: 'left'}
     ];
-    spyOn(reuseStrategy, 'retrieve').and.callThrough();
+    spyOn(reuseStrategy, 'retrieve');
 
     const prevState =
         createRouterState(reuseStrategy, createState(config, 'a(left:b)'), emptyState());
     advanceState(prevState);
-
-    // Expect 2 calls as the baseline setup
-    expect(reuseStrategy.retrieve).toHaveBeenCalledTimes(2);
-
-    // This call should produce a reused activated route
-    const state = createRouterState(reuseStrategy, createState(config, 'a(left:c)'), prevState);
-
-    // Verify the retrieve method has been called one more time
-    expect(reuseStrategy.retrieve).toHaveBeenCalledTimes(3);
+    createRouterState(reuseStrategy, createState(config, 'a(left:c)'), prevState);
+    expect(reuseStrategy.retrieve).not.toHaveBeenCalled();
   });
 
   it('should consistently represent future and current state', () => {

packages/router/test/integration.spec.ts

@@ -5807,6 +5807,7 @@ describe('Integration', () => {
          const fixture = createRoot(router, RootCmp);
 
          router.routeReuseStrategy = new AttachDetachReuseStrategy();
+         spyOn(router.routeReuseStrategy, 'retrieve').and.callThrough();
 
          router.resetConfig([
            {
@@ -5824,14 +5825,19 @@ describe('Integration', () => {
          expect(location.path()).toEqual('/a/b');
          expect(teamCmp).toBeDefined();
          expect(simpleCmp).toBeDefined();
+         expect(router.routeReuseStrategy.retrieve).not.toHaveBeenCalled();
 
          router.navigateByUrl('/c');
          advance(fixture);
          expect(location.path()).toEqual('/c');
          expect(fixture.debugElement.children[1].componentInstance).toBeAnInstanceOf(UserCmp);
+         // We have still not encountered a route that should be reattached
+         expect(router.routeReuseStrategy.retrieve).not.toHaveBeenCalled();
 
          router.navigateByUrl('/a;p=1/b;p=2');
          advance(fixture);
+         // We retrieve both the stored route snapshots
+         expect(router.routeReuseStrategy.retrieve).toHaveBeenCalledTimes(2);
          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');