[CLEANUP+BUGFIX] Refactor query params tests #14433
Conversation
trentmwillis
force-pushed
the
qp-tests
branch
2 times, most recently
from
9e79817 to
685f81f
Compare
trentmwillis
changed the title
|
This PR is ready to review now. There are a couple skipped tests / fixmes which I am unsure how to address. They've been brought up in #dev-router on Slack and will try to get resolution before moving forward on this. |
…alization Fixes a bug where QP values can collide on initial page visit
trentmwillis
force-pushed
the
qp-tests
branch
from
0af2cfb to
d1124fa
Compare
trentmwillis
changed the title
|
This should be ready to go now. I fixed the above issues based on discussions in Slack (documented as separate commits). Edit: I can break out the commits into separate PRs if needed/desired. |
| let queryParams = getQueryParamsFor(route, state); | ||
|
|
||
| return Object.keys(queryParams).reduce((params, key) => { | ||
| assert(`The route '${this.routeName}' has both a dynamic segment and query param with name '${key}'. Please rename one to avoid collisions.`, !params[key]); |
There was a problem hiding this comment.
Could people have gotten away with this before? New assertions smell like new public API that may need to get documented.
There was a problem hiding this comment.
Sort of. This assertion used to be done during _serializeQueryParams, which means that it caught the issue the majority of the time, but would miss it if landing on a route with colliding params first (since we didn't serialize the params, they were just given).
Additionally, this should be a slight performance win since this will cache after the first attempt to look up the QPs for a given route. Previously, the collision check happened for every serialization of every link.
There was a problem hiding this comment.
Oh nevermind, thought this was on a different part of the code. Yeah this is a new assertion, we can drop this if needed, but the behavior in this case has never really been defined
|
Thanks @trentmwillis! |
| return { | ||
| location: 'test' | ||
| }; | ||
| } |
There was a problem hiding this comment.
Do we require both instances of this? https://github.com/emberjs/ember.js/pull/14433/files#diff-673d09deaa2de598aaa24b970ea916a6R30
There was a problem hiding this comment.
Extend both from the same place?
There was a problem hiding this comment.
Will address this in my next PR.
This PR refactors the Query Param tests to use a setup similar to the
ember-glimmertests. It also adds a few tests that were identified as gaps in the original coverage. The primary goal is to provide a set of abstractions to make writing/maintaining these tests easier.The follow-up to this PR will be to better organize these tests so that we can more easily identify potential gaps in coverage.