From 3817e5f1dfeb9de26fa2ea4068a37565f435214f Mon Sep 17 00:00:00 2001
From: Mathieu Lemoine <github@mlemoine.name>
Date: Tue, 25 Sep 2018 11:18:31 -0400
Subject: [PATCH] fix(router): Fix arguments order for call to shouldReuseRoute
 (#26949)

The `createOrReuseChildren` function calls shouldReuseRoute with the
previous child values use as the future and the future child value used
as the current argument. This is incosistent with the argument order in
`createNode`. This inconsistent order can make it difficult/impossible
to correctly implement the `shouldReuseRoute` function. Usually this
order doesn't matter because simple equality checks are made on the
args and it doesn't matter which is which.

More detail can be found in the bug report: #16192.

Fix #16192

BREAKING CHANGE: This change corrects the argument order when calling
RouteReuseStrategy#shouldReuseRoute. Previously, when evaluating child
routes, they would be called with the future and current arguments would
be swapped. If your RouteReuseStrategy relies specifically on only the future
or current snapshot state, you may need to update the shouldReuseRoute
implementation's use of "future" and "current" ActivateRouteSnapshots.

PR Close #26949
---
 packages/router/src/create_router_state.ts    |  2 +-
 .../router/test/create_router_state.spec.ts   | 32 ++++++++++++++++++-
 2 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/packages/router/src/create_router_state.ts b/packages/router/src/create_router_state.ts
index 17e0f8b6ad..92ca55bdbf 100644
--- a/packages/router/src/create_router_state.ts
+++ b/packages/router/src/create_router_state.ts
@@ -65,7 +65,7 @@ function createOrReuseChildren(
     prevState: TreeNode<ActivatedRoute>) {
   return curr.children.map(child => {
     for (const p of prevState.children) {
-      if (routeReuseStrategy.shouldReuseRoute(p.value.snapshot, child.value)) {
+      if (routeReuseStrategy.shouldReuseRoute(child.value, p.value.snapshot)) {
         return createNode(routeReuseStrategy, child, p);
       }
     }
diff --git a/packages/router/test/create_router_state.spec.ts b/packages/router/test/create_router_state.spec.ts
index 3351eb2be9..7afb907eda 100644
--- a/packages/router/test/create_router_state.spec.ts
+++ b/packages/router/test/create_router_state.spec.ts
@@ -16,7 +16,10 @@ import {DefaultUrlSerializer, UrlSegmentGroup, UrlTree} from '../src/url_tree';
 import {TreeNode} from '../src/utils/tree';
 
 describe('create router state', () => {
-  const reuseStrategy = new DefaultRouteReuseStrategy();
+  let reuseStrategy: DefaultRouteReuseStrategy;
+  beforeEach(() => {
+    reuseStrategy = new DefaultRouteReuseStrategy();
+  });
 
   const emptyState = () =>
       createEmptyState(new (UrlTree as any)(new UrlSegmentGroup([], {}), {}, null!), RootComponent);
@@ -109,6 +112,33 @@ describe('create router state', () => {
     // Verify the retrieve method has been called one more time
     expect(reuseStrategy.retrieve).toHaveBeenCalledTimes(3);
   });
+
+  it('should consistently represent future and current state', () => {
+    const config = [
+      {path: '', pathMatch: 'full', component: ComponentA},
+      {path: 'product/:id', component: ComponentB}
+    ];
+    spyOn(reuseStrategy, 'shouldReuseRoute').and.callThrough();
+    const previousState = createRouterState(reuseStrategy, createState(config, ''), emptyState());
+    advanceState(previousState);
+    (reuseStrategy.shouldReuseRoute as jasmine.Spy).calls.reset();
+
+    createRouterState(reuseStrategy, createState(config, 'product/30'), previousState);
+
+    // One call for the root and one call for each of the children
+    expect(reuseStrategy.shouldReuseRoute).toHaveBeenCalledTimes(2);
+    const reuseCalls = (reuseStrategy.shouldReuseRoute as jasmine.Spy).calls;
+    const future1 = reuseCalls.argsFor(0)[0];
+    const current1 = reuseCalls.argsFor(0)[1];
+    const future2 = reuseCalls.argsFor(1)[0];
+    const current2 = reuseCalls.argsFor(1)[1];
+
+    // Routing from '' to 'product/30'
+    expect(current1._routerState.url).toEqual('');
+    expect(future1._routerState.url).toEqual('product/30');
+    expect(current2._routerState.url).toEqual('');
+    expect(future2._routerState.url).toEqual('product/30');
+  });
 });
 
 function advanceState(state: RouterState): void {