Calling recognize should not affect the transition.from query params for subsequent transitions #336
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…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
suggested changes
Dec 8, 2023
package-lock.json
Outdated
NullVoxPopuli
approved these changes
Jan 25, 2024
|
@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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.