[Bug] Chained redirects triggered via `routeWillChange` lead to calling model() hook with wrong params

[davidtaylorhq]

🐞 Describe the Bug

When there is a chain of two redirects triggered via a routeWillChange hook, the destination route model() hook is called twice, and is called with the wrong parameters.

🔬 Minimal Reproduction

// Route config:
this.route('dynamic', { path: '/dynamic/:dynamic_id' });

// RouteWillChange hook:
routerService.on('routeWillChange', (transition) => {
  let to = transition.to;

  if (to.name === 'dynamic') {
    if (to.params.dynamic_id === '1') {
      this.routerService.transitionTo('dynamic', '2');
    } else if (to.params.dynamic_id === '2') {
      this.routerService.transitionTo('dynamic', '3');
    }
  }
});

Visiting /dynamic/1 will redirect to /dynamic/3, but the model hook will be called (twice) with dynamic_id=2.

Failing test case in https://github.com/emberjs/ember.js/pull/20612

😕 Actual Behavior

The model hook will be called (twice) with dynamic_id=2.

🤔 Expected Behavior

The model hook should be called once with dynamic_id=3

🌍 Environment

Tested under Ember 3.28 and 5.6.0.beta2

[void-mAlex]

hello @davidtaylorhq would you be able to provide more info on the expected behaviour? a link to a RFC that described that behaviour or documentation to that effect or the use case that you have

I can also see the thinking behind the current behaviour where doing a redirect outside the beforeModel hook would end up calling it twice as routeWillChange happens after the model hook is called and the transition is considered settled as per the docs here https://api.emberjs.com/ember/release/classes/routerservice/events/ so I would not call this a bug

[davidtaylorhq]

routeWillChange happens after the model hook is called

Perhaps you're thinking of routeDidChange?

My understanding is that routeWillChange is fired before any of the route model hooks (beforeModel, model, afterModel). Per the docs, the event is intended for 'aborting, redirecting, or decorating the transition', all of which needs to happen before the model() hook.

Even if we decided the double-call of the model() hook is intentional, it certainly shouldn't be called with the wrong param the second time.

The failing test illustrates both the double-call and the incorrect-param issue. ('willchange-to' is logged in the routeWillChange hook, loaded- is logged in the route's model() function)

Failed assertion: expected:
  willchange-to-1,
  willchange-to-2,
  willchange-to-3,
  loaded-3

but was:
  willchange-to-1,
  willchange-to-2,
  willchange-to-3,
  loaded-2,
  loaded-2

[void-mAlex]

in your example you're never aborting the current transition when redirecting to id 3 or am I missing something?

[davidtaylorhq]

in your example you're never aborting the current transition

That's true. And indeed, if I add manual transition.abort() calls before each redirect, my test case succeeds.

But still, this requirement is quite unexpected. In the redirection docs it says that calling transitionTo or replaceWith will implicitly abort the the active transition. Also in the Transition API docs it also says

you can also implicitly abort a transition by initiating another transition while a previous one is underway.

So perhaps the bug report here should really be "Transitions are not implicitly aborted when chaining multiple redirects via routeWillChange". What do you think?

[void-mAlex]

I honestly would never expect that to work from an event but rather via initiating a new transition from a beforeModel hook the way I think about this is the event is for knowing that something happeneded or will happen and the hook is for interfearing with it and redirecting the docs you linked show that happening from the hooks not the service event

and someone with knowledge of the intentions of the code here should correct me if what I'm saying is wrong

[kategengler]

I agree that I would have used a hook on the parent route rather than the router service event to accomplish the same thing. I think the details here may help https://guides.emberjs.com/release/routing/redirection/#toc_child-routes

I also suspect that this is is a slight different in routerService.transitionTo vs router.transitionTo (but have not verified). We are unlikely to change any router code at this point (any bug we fix breaks someone else relying upon said bug), so for now, I would either refactor to use beforeModel or redirect hooks on the parent route or call transition.abort().