-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[3.6+] Query Params behavior changes with RouterService when refreshModel
is true
#18683
Comments
Hey good news, this is more complicated! The same buggy behavior shows up when A plain Best i can tell, this is caused by over-applying the "don't strip default values" rule established in the RFC -- ember.js/packages/@ember/-internals/routing/lib/system/route.ts Lines 2584 to 2585 in c250929
While it is important not to instantiate controllers unnecessarily, all the relevant controllers are already there by that point in This prompts me to wonder if the RFC was mis-interpreted. There are tests that enforce default values are still serialized to the URL post-transition -- is that what we actually want? edit: adding a gif of the repro above in action (and added a description to the repro readme): |
workaround for emberjs/ember.js#18683
Looks quite similar to #18268 |
Unsure if this is the same issue we are seeing as well, but we're trying to update from Ember 3.12 to 3.14 and we're seeing some weirdness where queryParams now seem to get reset when calling |
@rwwagner90 Was also seeing some issues with queryParams upgrading from 3.12 to 3.13. |
@rwwagner90 @jakebixbyavalara can you open that up as a separate issue? It's likely related to the changes introduced in 3.13, which would be unrelated to this issue since it has been occuring since 3.6. |
Oh, definitely was going to open another issue, I just noticed that @rwwagner90 was mentioning 3.12 to 3.14 which I would assume is related to my issue and not this one. |
@jakebixbyavalara can you link to the issue after you open it? Definitely think 3.13 was what broke some thing for us. |
@aaxelb / @chriskrycho - I'm looking into this issue now. @aaxelb - I'm using your reproduction repository, but the tests don't seem to be failing where I expect. Is there a test in that repo (https://github.com/aaxelb/repro-ember-router-service-qp) that shows the failing issue? Or is there a specific step in the app I'm supposed to take? Pretty new to investigating Ember bugs (as in, this is my first), so any details are appreciated. |
@richgt ah sorry, there aren't tests, it's an interactive repro showing the problem when
i didn't write a test because ember's tests currently enforce the "incorrect" behavior, and maybe i'm wrong about how it should work? |
@aaxelb - perfect. thanks for the details. I think in general, it would be helpful for stuff like this to be in the README so that as folks come in to look at the issue, they know exactly how to repro :-D. |
@richgt @aaxelb I've tested this repo against ember 3.25.1, and this behavior is still here. There is definitely something inconsistent with the service, because using (the now deprecated transitionToRoute()) from the controller, it has the same behavior as link-to. Reading through the code, I felt on https://github.com/emberjs/rfcs/blob/master/text/0095-router-service.md#query-parameter-semantics, so the behavior seems expected. Would that mean the other behaviors are wrong and should be fixed to make things consistent ? (the usual transitionTo are deprecated after all...) cc/ @rwjblue @chancancode |
Yes, I think the older behaviors should be deprecated if we can figure out how to do it reasonably. |
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.When query params do not have
refreshModel: true
set, usingRouterService#transitionTo
ultimately results in query params being removed from the URL once the target route resolves. However, ifrefreshModel: 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
leadsRoute#queryParamsDidChange
to invokeRoute#refresh
:ember.js/packages/@ember/-internals/routing/lib/system/route.ts
Lines 2495 to 2512 in d96d9aa
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 invokingRoute#refresh
creates a newTransition
, 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 triggeringfireQueryParamsDidChange
(router_js.js
)queryParamsTransition
(router_js.js
)getTransitionByIntent
(router_js.js
)The
getTransitionByIntent
method calculates aqueryParamChangelist
which is then supplied to the rest of the chain, but has insufficient information to correctly generate thatqueryParamChangelist
: 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) prunes default query params from the list of query params—it only skips that operation when the transition comes from the router service:ember.js/packages/@ember/-internals/routing/lib/system/router.ts
Lines 902 to 904 in d96d9aa
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. 😬
The text was updated successfully, but these errors were encountered: