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.

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

[BUGFIX beta] Improve behavior for QPs with undefined values #14537

Merged
merged 1 commit into from
Oct 28, 2016

Conversation

trentmwillis
Copy link
Member

For the case of lazy loading Engines, all QPs get serialized into the URL initially. undefined shouldn't get serialized to a string, so that when it gets pulled out the value is not a string.

cc @nathanhammond

@nathanhammond
Copy link
Member

nathanhammond commented Oct 27, 2016

People willing to make hard decisions: can we take this a step further? I'd like to force all values passed into queryParams to be JSON serializable.

 _serializeQueryParams(handlerInfos, queryParams) {
  queryParams = JSON.parse(JSON.serialize(queryParams));
  // ...

This makes it possible for people to either pass arbitrary objects which implement toJSON as values to keys and also sets us up well for a world in which we do structured query params.

It also neatly sidesteps this issue:

JSON.parse(JSON.stringify({foo:undefined}));
// => {}
JSON.parse(JSON.stringify({foo:null}));
// => { foo: null }

Thoughts?

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Looks good to me, with one small nit-pick...

@@ -669,7 +669,7 @@ const EmberRouter = EmberObject.extend(Evented, {
if (qp) {
delete queryParams[key];
queryParams[qp.urlKey] = qp.route.serializeQueryParam(value, qp.urlKey, qp.type);
} else {
} else if (value !== undefined) {
Copy link
Member

Choose a reason for hiding this comment

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

Super not-picky but I'd rather leave this as an unconditional else clause. Could you swap this to check if it's undefined and just noop (as an else if before the final else)?

@rwjblue
Copy link
Member

rwjblue commented Oct 28, 2016

@nathanhammond - Let's keep that separate from this PR, but generally I think it could work (generally speaking). Happy to review a PR (or general sketch of the logic).

@trentmwillis
Copy link
Member Author

@rwjblue updated, but getting a linting error. Should I just return from the block?

@rwjblue rwjblue merged commit d37d38b into emberjs:master Oct 28, 2016
@rwjblue
Copy link
Member

rwjblue commented Oct 28, 2016

Thanks @trentmwillis!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants