Calling recognize should not affect the transition.from query params for subsequent transitions#336
Conversation
…for subsequent transitions
…nally using a local map for toReadOnlyRouteInfo. Always using a local map for toReadOnlyRouteInfo causes many tests to fail so apparently it is expected to polute state in many instances.
tests/router_test.ts
Outdated
| }) | ||
| .then(() => { | ||
| secondParam = true; | ||
| router.recognize('/search?term=foo'); |
There was a problem hiding this comment.
This test isn't actually convincing me that the query params aren't mutated, because they use the same qp.
If recognize was used with a query param that wasn't term, then I would be convinced!
There was a problem hiding this comment.
@NullVoxPopuli query param has been updated
NullVoxPopuli
left a comment
NullVoxPopuli
left a comment
There was a problem hiding this comment.
Thanks a ton for working on this, I think the change is fine, and given that folks will / should be using recognize for determining "active" state for their links, this is a great fix.
package-lock.json
Outdated
NullVoxPopuli
left a comment
NullVoxPopuli
left a comment
There was a problem hiding this comment.
I don't have access to merge and release, but this seems fine
|
@NullVoxPopuli looks like @wagenet had release on this a few years back, might be worth seeing if he could? |
wagenet
force-pushed
the
recognize-qp-issue
branch
from
730badb to
9a61340
Compare
|
I also released this as 8.0.4. |
|
Update to Ember: emberjs/ember.js#20656 |
4 participants
Problem Statement
Calling router.recognize with a URL which matches the current route affects the transition.from.queryParams for subsequent transitions. I've created a failing test here.
Root Cause
The root cause appears to be caused by the fact that
toReadOnlyRouteInfo(source) closes over a WeakMap forROUTE_INFOSwhich is then mutated on subsequent calls (including calls from router.recognize).Proposed Solution
This PR adds a new argument to
toReadOnlyRouteInfo,localizeMapUpdates, which allows the function to conditionally use a locally scoped map instead of the map from the parent scope. Interestingly, always using a locally scoped map intoReadOnlyRouteInfocauses a number of tests to fail.recognizewas then updated to enable the new option (see here).This proposal passes this project's test suite and a large production Ember app's test suite.