RouteReuseStrategy.shouldReuseRoute has arguments swapped

[poke]

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.

[geetee24]

I concur with poke. it is not yet fixed at least in v 4.1.1

[compeek]

I just noticed this on 4.3.6 as well and then found this issue to confirm I'm not the only one.

[arlowhite]

Still not fixed in 5.2.3
The two arguments at the call site just need to be switched right?
@vsavkin Could you double-check this line?

angular/packages/router/src/create_router_state.ts

Line 65 in 3a86940

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

[sliekens]

I can confirm this is wrong. I'm trying to implement a route reuse strategy for a named router outlet and I'm getting swapped curr and future arguments for the named outlet.

Right now I'm doing something like this to work around the issue.

shouldReuseRoute(future: ActivatedRouteSnapshot, curr: ActivatedRouteSnapshot): boolean {
    if (future.outlet !== 'primary') {
        // do the switcheroo
        [future, curr] = [curr, future];
    }
    if (curr.routeConfig && curr.routeConfig.data && curr.routeConfig.data.useOnce) {
        return false;
    } else {
        return future.routeConfig === curr.routeConfig;
    }
}

[lemoinem]

I have a trivial fix, but I'm not sure what tests need to be added so the PR is acceptable:
lemoinem@deaeb3e

I want to note that inverting the two arguments is not enough:
createOrReuseChildren invokes the method in the wrong order and then calls createNode which invokes the method again, but with the arguments in the correct order this time.

Basically, any asymmetric routeReuseStrategy is impossible because of this issue.
How come this hasn't been fixed in more than a year and a half?

[sliekens]

@lemoinem my workaround for that is to swap the arguments only if the first snapshot targets the 'primary' outlet. This guarantees that the call did not originate from createOrReuseChildren.

Of course that workaround will break when createOrReuseChildren gets fixed.

[lemoinem]

@StevenLiekens This doesn't work for me.
I am looking at routes in the primary outlets. And the methods is invoked once with the arguments in the correct order and once with the arguments in the wrong order. With the exact same arguments.

[sliekens]

Weird I thought only child routes were affected by the swapped arguments bug

[liutian]

Still not fixed in 7.2.7

[gabriele-fanchini]

versione 7.2.10 still effected. It would be great to have some news about it...

[liutian]

Still not fixed in 8.2.3

[lemoinem]

There is a one-line fix PR waiting to be reviewed/merged, but I had no feedback. I'm open to any suggestion as to what to do to get it merged or attract some kind of attention.

[skaindl]

I just ran into this as well. I find it rather sad that this bug is open so long, when someone just has to change a single line to fix a major inconsistency.

@jasonaden @mhevery, could someone please have a look at the PR for this?

[samlanzz]

Any news on this? I seem to be losing my route.children along the way due to this problem.

[atscott]

Hi all,

I agree that the code looks suspect and is likely incorrect. That said, it's difficult to justify changing anything without a proper demonstration of incorrect behavior. Can someone create a minimal repro on StackBlitz that demonstrates unexpected behavior due to this bug?

[johnny-mh]

@atscott
https://stackblitz.com/edit/angular-ivy-crrs-swapped
You can check the future and curr swapped by clicking settings detail link.
shouldReuseRoute invoke twice. 1st is swapped future and curr ambiguously.

[Pupskuchen]

I know this has been fixed, but since that fix hasn't been released yet I had to come up with a workaround to get a consistent order of arguments. Now I haven't used this for a long time, but so far it seems like it's doing its job pretty … consistently?

So I thought I'd share it with you, maybe it'll be helpful for someone or you have a different idea. In the leaderboard of worst code I've written, this is ranked pretty high:

private createNodeStack: string;
public shouldReuseRoute(future: ActivatedRouteSnapshot, curr: ActivatedRouteSnapshot): boolean {
    const caller = new Error().stack.split('\n')[1];

    /*
     * See comment below for why we do this.
     * When called for the first time, store caller from stack trace.
     * The first call will be from 'createNode'.
     */
    if (!this.createNodeStack) {
        this.createNodeStack = caller;
    }

    /*
     * Workaround for Angular bug #16192 (https://github.com/angular/angular/issues/16192)
     * Argument order for calls of this function is currently not consistent so
     * we need to swap them if called from 'createOrReuseChildren'.
     * This will break in a future release when the order is fixed.
     */
    if (caller !== this.createNodeStack) {
        [future, curr] = [curr, future];
    }

    […] // do your thing here
}

[angular-automatic-lock-bot]

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}