Skip to content
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.

Sign up for GitHub

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

[CLEANUP+BUGFIX] Refactor query params tests #14433

Merged
merged 4 commits into from
Oct 12, 2016
Merged

[CLEANUP+BUGFIX] Refactor query params tests #14433

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
553383c
[CLEANUP] Refactor query params tests
Oct 7, 2016
bf619dc
[BUGFIX] Throw if dynamic segment and query param collide
Oct 10, 2016
d1124fa
[BUGFIX] Check for QP urlKey collisions during lookup instead of seri…
Oct 11, 2016
ded03d0
[CLEANUP] Clarify refreshModel + loading action behavior
Oct 11, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions packages/ember-routing/lib/system/route.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,14 +375,15 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
let transition = this.router.router.activeTransition;
let state = transition ? transition.state : this.router.router.state;

let params = {};
let fullName = getEngineRouteName(getOwner(this), name);

assign(params, state.params[fullName]);

assign(params, getQueryParamsFor(route, state));

return params;
let params = assign({}, state.params[fullName]);
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]);
Copy link
Contributor

@ef4 ef4 Oct 11, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could people have gotten away with this before? New assertions smell like new public API that may need to get documented.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

params[key] = queryParams[key];
return params;
}, params);
},

/**
Expand Down
34 changes: 16 additions & 18 deletions packages/ember-routing/lib/system/router.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -654,26 +654,10 @@ const EmberRouter = EmberObject.extend(Evented, {
},

_serializeQueryParams(targetRouteName, queryParams) {
let groupedByUrlKey = {};

forEachQueryParam(this, targetRouteName, queryParams, function(key, value, qp) {
let urlKey = qp.urlKey;
if (!groupedByUrlKey[urlKey]) {
groupedByUrlKey[urlKey] = [];
}
groupedByUrlKey[urlKey].push({
qp: qp,
value: value
});
delete queryParams[key];
queryParams[qp.urlKey] = qp.route.serializeQueryParam(value, qp.urlKey, qp.type);
});

for (let key in groupedByUrlKey) {
let qps = groupedByUrlKey[key];
assert(`You're not allowed to have more than one controller property map to the same query param key, but both \`${qps[0].qp.scopedPropertyName}\` and \`${qps[1] ? qps[1].qp.scopedPropertyName : ''}\` map to \`${qps[0].qp.urlKey}\`. You can fix this by mapping one of the controller properties to a different query param key via the \`as\` config option, e.g. \`${qps[0].qp.prop}: { as: \'other-${qps[0].qp.prop}\' }\``, qps.length <= 1);
let qp = qps[0].qp;
queryParams[qp.urlKey] = qp.route.serializeQueryParam(qps[0].value, qp.urlKey, qp.type);
}
},

_deserializeQueryParams(targetRouteName, queryParams) {
Expand Down Expand Up @@ -750,6 +734,7 @@ const EmberRouter = EmberObject.extend(Evented, {
return this._qpCache[leafRouteName];
}

let qpsByUrlKey = {};
let map = {};
let qps = [];
this._qpCache[leafRouteName] = {
Expand All @@ -767,8 +752,21 @@ const EmberRouter = EmberObject.extend(Evented, {

if (!qpMeta) { continue; }

// Loop over each QP to make sure we don't have any collisions by urlKey
for (let i = 0; i < qpMeta.qps.length; i++) {
let qp = qpMeta.qps[i];
let urlKey = qp.urlKey;

if (qpsByUrlKey[urlKey]) {
let otherQP = qpsByUrlKey[urlKey];
assert(`You're not allowed to have more than one controller property map to the same query param key, but both \`${otherQP.scopedPropertyName}\` and \`${qp.scopedPropertyName}\` map to \`${urlKey}\`. You can fix this by mapping one of the controller properties to a different query param key via the \`as\` config option, e.g. \`${otherQP.prop}: { as: \'other-${otherQP.prop}\' }\``, false);
}

qpsByUrlKey[urlKey] = qp;
qps.push(qp);
}

assign(map, qpMeta.map);
qps.push.apply(qps, qpMeta.qps);
}

return {
Expand Down
Loading