RouteReuseStrategy.shouldReuseRoute has arguments swapped #16192

Hey, I noticed the following inconsistency regarding the arguments passed to RouteReuseStrategy.shouldReuseRoute in create_router_state.ts:

In line 26, the function is called like this:

routeReuseStrategy.shouldReuseRoute(curr.value, prevState.value.snapshot)

So the first argument is the current state, and the second argument is the one before. This matches the documentation.

However, a few lines below, the following call is made:

const children = createOrReuseChildren(routeReuseStrategy, curr, prevState);

So here, curr and prevState are passed to createOrReuseChildren which in turn does the following:

function createOrReuseChildren(
    routeReuseStrategy: RouteReuseStrategy, curr: TreeNode<ActivatedRouteSnapshot>,
    prevState: TreeNode<ActivatedRoute>) {
  return curr.children.map(child => {
    for (const p of prevState.children) {
      if (routeReuseStrategy.shouldReuseRoute(p.value.snapshot, child.value)) {
        return createNode(routeReuseStrategy, child, p);
      }
    }
    return createNode(routeReuseStrategy, child);
  });
}

Here are two iterations happening, once of the children of curr and once for the children of prevState. Within the inner iteration, child is the child of curr, and p is the child of prevState. However, the call to shouldReuseRoute now is in reverse to the one before:

routeReuseStrategy.shouldReuseRoute(p.value.snapshot, child.value)

So first, the previous state child is passed and then the current state child, which is the reversed order to the function definition, where the second argument is the older state.

Now I’m not perfectly sure if this isn’t the desired behavior, since I don’t completely understand how the reuse strategy works, but this appears very odd to me. And it would explain the odd behavior I see when trying to implement a reuse strategy that dynamically decides based on the route. Right now, when switching from route A to B, I get two calls to shouldReuseRoute, once with A => B and once B => A making it impossible to create a strategy handle only one direction.

If someone confirms that this is a bug, I can gladly provide a pull request for the fix.