[3.6+] Query Params behavior changes with RouterService when `refreshModel` is `true`
- Dominant language
- TypeScript
- Stars
- 22.6k
- Forks
- 4.2k
- Avg merge
- 3d 12h
- Merged PRs (30d)
- 15
Description
While upgrading our application from Ember 3.4 to 3.8, we discovered that the behavior of query params in the URL changes *dramatically* depending on whether any of the query params have `refreshModel: true` set on the route. I created a sample example reproduction in [this repo](https://github.com/chriskrycho/weird-qps).
When query params do *not* have `refreshModel: true` set, using `RouterService#transitionTo` ultimately results in query params being removed from the URL once the target route resolves. However, *if* `refreshModel: true` is set for any of the query params, the query params remain in the URL once the target route resolves. While in principle this should always still work in a well-behaved application, it is definitely not desirable behavior (because not all applications *are* well behaved here!).
The issue is *surfaced* because the presence of `refreshModel` leads `Route#queryParamsDidChange` to invoke `Route#refresh`:
https://github.com/emberjs/ember.js/blob/d96d9aad52e010da977d813f479494397ee53ec6/packages/%40ember/-internals/routing/lib/system/route.ts#L2495-L2512
However, this is not the actual *cause*, just the surfacing symptom. The actual *cause* is that `queryParamsDidChange` is itself invoked with incorrect parameters. (It surfaces the issue clearly because invoking `Route#refresh` creates a new `Transition`, which carries along those query params.)
The call stack is:
- `queryParamsDidChange` (`router.js`)
- `triggerEvent` with event named `"queryParamsDidChange"` (`router.js`)
- `triggerEvent` with event named `"queryParamsDidChange"` (`router.js`) – this is not a mistake, there are two layers of event triggering
- `fireQueryParamsDidChange` (`router_js.js`)
- `queryParamsTransition` (`router_js.js`)
- `getTransitionByIntent` (`router_js.js`)
The `getTransitionByIntent` method [calculates a `queryParamChangelist`](https://github.com/tildeio/router.js/blob/bedbf4e2c6a544bc8a678d286d00b412d0ebab51/lib/router/router.ts#L210) which is then supplied to the rest of the chain, but has insufficient information to correctly generate that `queryParamChangelist`: it only knows the *actual* previous state of the query params for the route and the *new* query params… but (correctly) has no knowledge of Ember's Controller query params special handling.
This does not fail in the non-router-service case because when the transition comes from a route or a controller, `_prepareQueryParams` (again, correctly, [per the RFC][RFC]) prunes default query params from the list of query params—it only skips that operation when the transition comes from the router service:
https://github.com/emberjs/ember.js/blob/d96d9aad52e010da977d813f479494397ee53ec6/packages/%40ember/-internals/routing/lib/system/router.ts#L902-L904
[RFC]: https://emberjs.github.io/rfcs/0095-router-service.html#query-parameter-semantics
Thus, the list of query params passed on down the chain still includes *all* values, whether or not they are defaults… so what ends up being checked by the the router microlib for different values includes those values.
At first blush, *none* of the changes we could make in this space are obvious winners, or even particularly good. :grimacing:
Contributor guide
Assessment
This issue has not been assessed yet.