emberjs / emberjs/ember.js

[3.6+] Query Params behavior changes with RouterService when `refreshModel` is `true`

Open
#18,683 12 comments 0 reactions 0 assignees View on GitHub
Has Reproduction Query Params Routing
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

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.