[BUGFIX] normalize query params when merging in from active transition #13682
Conversation
fivetanley
force-pushed
the
moar-qp-fixes
branch
from
0c2d816
to
0a05227
Compare
| @@ -2490,6 +2491,43 @@ if (isEnabled('ember-routing-route-configured-query-params')) { | |||
| bootApplication(); | |||
| }); | |||
|
|
|||
| QUnit.test('queryParams are updated when a controller property is set and the route is refreshed. Issue #13263 ', function() { | |||
| Ember.TEMPLATES.application = compile( | |||
There was a problem hiding this comment.
Should be using setTemplate here.
|
☔ The latest upstream changes (presumably #13658) made this pull request unmergeable. Please resolve the merge conflicts. |
fivetanley
force-pushed
the
moar-qp-fixes
branch
2 times, most recently
from
c6b8865
to
5f21c25
Compare
This merges in queryParams from an active transition in such a way that
`prepareQueryParams` won't duplicate them. It also won't merge in the
qp if it is already int he qp changed list, for example if it's already
been set during route setup by a call to `controller.set('qpProp',
'someValue')`. Fixing these things with my current test jsbin also
recreated another issue that has been biting people and that is that a
call to `transitionTo` creates an aborted transition that calls
`didTransition` with an empty set of handlerInfos. This would cause the
`updatePaths` method to fail since it couldn't calculate the paths
without the handler infos. We now bail early from `updatePaths` it is
called with an empty array of handlerInfos. I think this is fine
because the aborted transition is followed by a successful transition
that will update the paths appropriatly.
This gets the active queryParams and the provided using the same key
`controller:prop` so that the calls to `assign` override as expected.
This fixes issues where the QP doesn't update properly on refresh
because the merged value conflicts with the provided value.
fivetanley
force-pushed
the
moar-qp-fixes
branch
from
5f21c25
to
d4dc6dd
Compare
fivetanley
force-pushed
the
moar-qp-fixes
branch
from
d4dc6dd
to
adef2d0
Compare
| @@ -999,6 +1044,8 @@ function calculatePostTransitionState(emberRouter, leafRouteName, contexts) { | |||
|
|
|||
| function updatePaths(router) { | |||
| let infos = router.router.currentHandlerInfos; | |||
| if (infos.length === 0) { return; } | |||
There was a problem hiding this comment.
I added this in to try and fix the same issue that this was trying to resolve:
#13426
Looks like that PR got abandoned, and I've lost my mental model of what was going on, but I was blocked on trying to create a test that could reproduce it. I'm wondering if all the tests pass without this line, if maybe we should just pull it?
|
I commented on the one line that I'm not sure should be there any longer since I was never able to create a test for it. Other than that this looks like what I intended. Thanks everyone for picking up where I left off. |
|
@fivetanley - Failing with optional tests enabled. You can test locally via: |
|
@fivetanley thanks for pushing to finally fix this. I took your rebase and fixed the tests: in #13273 |
rebased version of #13273, plus added test from @igorT.
From the original PR/commit message (i lazily squashed them into one, @raytiley may want to help edit):
Normalize query params when merging in from active transition
This merges in queryParams from an active transition in such a way that
prepareQueryParamswon't duplicate them. It also won't merge in theqp if it is already int he qp changed list, for example if it's already
been set during route setup by a call to
controller.set('qpProp', 'someValue'). Fixing these things with my current test jsbin alsorecreated another issue that has been biting people and that is that a
call to
transitionTocreates an aborted transition that callsdidTransitionwith an empty set of handlerInfos. This would cause theupdatePathsmethod to fail since it couldn't calculate the pathswithout the handler infos. We now bail early from
updatePathsit iscalled with an empty array of handlerInfos. I think this is fine
because the aborted transition is followed by a successful transition
that will update the paths appropriatly.
This gets the active queryParams and the provided using the same key
controller:propso that the calls toassignoverride as expected.This fixes issues where the QP doesn't update properly on refresh
because the merged value conflicts with the provided value.