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 release] Fix array QP serialization #14171

Closed
wants to merge 1 commit into from

Conversation

lan0
Copy link
Contributor

@lan0 lan0 commented Aug 31, 2016

A twiddle reproducing the issue: https://ember-twiddle.com/c0f6b0efd71731ecb7e474ddedc7a7a4 - after a click on set array and then on refresh the array QP becomes a string.

"[1]" gets stringified in the second call to serializeQueryParam, resulting in the serialized param being ""[1]"" instead of "[1]".

This fixes #13591 and is similar to #13816.

`"[1]"` gets stringified in a subsequent call to `serializeQueryParam`, resulting in the serialized param being `""[1]""` instead of `"[1]"`.

This fixes emberjs#13591 and is similar to emberjs#13816.
@nathanhammond
Copy link
Member

@lan0 I believe that both this approach and the original behavior are wrong. Per our adoption of route-recognizer and the way it handles arrays the result should be:

QS.stringify({
  array: [ 1 ]
});
// => ?array[] = 1

@nathanhammond
Copy link
Member

So these are the lines which implement the behavior:
https://github.com/emberjs/ember.js/blob/master/packages/ember-routing/lib/system/route.js#L421-L464

This is a completely custom (and broken) QP serialization strategy which doesn't match our approach for route-recognizer. The behavior for non-string and non-numeric values should be considered broken since who knows when and therefore should not be relied upon.

@nathanhammond
Copy link
Member

Last note: this should be compared to route-recognizer's current QP implementation:

The real bug here is actually that these are inconsistent. As a result we should not merge this PR (which will create yet another new failure mode) and instead solve it at the more-fundamental level.

Blog post coming on query strings this week.

@homu
Copy link
Contributor

homu commented Oct 4, 2016

☔ The latest upstream changes (presumably #14415) made this pull request unmergeable. Please resolve the merge conflicts.

@lan0
Copy link
Contributor Author

lan0 commented Oct 12, 2016

Agreed that the array serialization should be ?array[]=, but it worked for us so we rolled with it long ago.

Closing this since we fixed it in our app by overwriting serializeQueryParam in routes where there are array QP:

  serializeQueryParam(value, urlKey, defaultValueType) {
    if (defaultValueType === 'array' && Array.isArray(value)) {
      return JSON.stringify(value);
    }
    return `${value}`;
  },

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.

_array_ queryParam converted to _string_ after transitionTo
3 participants