TransitionTo with queryParams refreshModel triggers model calls on other routes

[Boudewijn26]

Description

It appears that calling transitionTo with queryParams on a nested route can trigger calls to the lower route if that route has a refreshModel: true on an unchanged queryParam. Reproduction

So I have this setup:
somewhere -> somewhereParam with refreshModel: true with default null
somewhere.nested -> nestedParam

In somewhere.nested I have a component which triggers a transition on the router service where nestedParam is changed. This trigger the model on somewhere to be called and then transitioning through the controller then triggers another call.

Cause

I did some digging and it appears that the call to route#finalizeQueryParamChange returns the incorrect values, because the transition._keepDefaultQueryParams isn't set. This is set in router#transitionTo, but only after the call. The call does return a promise, so transition._keepDefaultQueryParams = true may actually be executed in time, but I found this to be very flaky.

I'm more than willing to help fix this, however it is embedded so deeply that I find it difficult to pick the right approach.

[LiquidSean]

@Boudewijn26 so it appears like in previous versions of ember ( < 2.16.2) The model is only hit once. So the value of 'calls' should always be 1. That is what I am seeing the intended results should be.

@Boudewijn26 I did some digging and you are right it is embedded fairly deep and picking a right approach is tough. I see what needs to be done just not sure where to start.

[rwjblue]

@Boudewijn26 - Thanks for opening the issue! I'd love to figure this out with you, I think the first step here would be to submit a failing test PR. The tests for the router service around query params are here...

[rwjblue]

One thing that I'm not certain of here, is if this is an actual bug in the implementation (thats why a failing test PR would be super helpful) or a bug in the documentation around the router service.

This invocation is what concerns me:

// from the component in `somewhere.nested` the router service transitionTo invocation is:

this.get('router').transitionTo({
  queryParams: {
    nestedParam: 'a'
  }
})

With the current implementation of the router service, only the specified query params will be used. From the perspective of the implementation, this implicitly means that you are setting somewhereParam to undefined (which due to refreshModel: true is triggering the model hook to be invoked again).

What we need to figure out is if the current implementation is wrong (e.g. if the RFC says that we should inherit existing query params) or if the docs are wrong (e.g. that this is expected behavior but not well documented)...

[LiquidSean]

@rwjblue I did notice in ember-cli version 2.13.3 that the param somewhereParam was cleared so there were never any changes so the model hook was never invoked again. In ember-cli version 2.16.2 I noticed that somwhereParam was held onto causing there to be changes and the model hook being invoked.

[LiquidSean]

@rwjblue The line in ember-routing I am looking at that I am not entirely sure about is
--> [https://github.com/emberjs/ember.js/blob/master/packages/ember-routing/lib/system/router.js#L976]

For somwhereParam the default value is null which is expected but should 'somewhereParam' really be set? My idea is that it should be removed from the query params list unless it really has been changed since it is still the default value there is not reason to include it in the list of changes. Perhaps I am not understanding the motivation for it entirely.

[Boudewijn26]

@rwjblue I'll get to work on those tests.

For me the expected behavior would be following the implementation of transitionToRoute from controllers and keeping the existing query params. Also if somewhereParam were actually to be set to undefined, then the value would need to change. I've updated the reproduction to display the value of somewhereParam and it doesn't change. Also if it where to change, then it should stay undefined, and triggering another router service transition, shouldn't trigger another model call. Also the setting of _keepDefaultQueryParams points to an attempt to keep the default query params.

[Boudewijn26]

@rwjblue I have opened #15832, would you mind taking a look and giving pointers on where to start fixing this?

[Boudewijn26]

@rwjblue Sorry to bother you, but this is still open. Would it be possible for you or anyone else to give some pointers on how best to fix this?

[Serabe]

@rwjblue RFCS states that transitionTo and replaceWith should behave as previous methods in Ember.Route, but documentation for those methods does not clarify the behaviour for several query params.

[Boudewijn26]

@rwjblue This bug is currently 50 days old. While I understand effort is currently going to Ember 3.0, I hope we can get this fixed within reasonable time for the LTS releases.

[yratanov]

@rwjblue will this bug be fixed? It makes router service unusable.

[wuarmin]

I encountered a similar issue.
I have following queryParams-definition:

//controller
queryParams: ['createWith'],
createWith: null

//route
queryParams: {
  createWith: {
    refreshModel: true
  }
},

If I use the router service and call

this.router.transitionTo('content-blocks.create', {
  queryParams: {
    createWith: null
  }
});
//or
this.router.transitionTo('content-blocks.create');

the model hook of route 'content-blocks.create' is called twice.

---

I found a temporary workaround:

this.router.transitionTo('content-blocks.create', {
  queryParams: {
    createWith: undefined //this works
  }
});

[rmachielse]

@rwjblue can this get some attention? This makes the router service unusable in combination with refreshModel: true since ember 2.15.

[kurkowski]

I just came across this bug on Ember 3.3. Using the router service's transitionTo method in a component would cause the transitioned-to route's model hook to be called twice. My workaround was to pass an action defined in the controller down to the component and use the controller's transitionToRoute method which does not seem to have the bug.