[BUGFIX release] Prevent multiple qp serialization on activeTransition #19236
Conversation
...on activeTransition in `_doTransition`. `_processActiveTransitionQueryParams` returns serialized values and we serialize them again in `_prepareQueryParams`, which expects deserialized values. Refs: emberjs#14174
We're using serialized query parameters for active transition instead of deserialized ones. This results in multiple serialization of QP's.
rreckonerr
force-pushed
the
fix/active-transition-qp-serialization
branch
from
f8ed040 to
20da5b2
Compare
rreckonerr
changed the title
| ['@test Calling transitionTo does not serialize query params already serialized on the activeTransition']( | ||
| assert | ||
| ) { | ||
| assert.expect(3); | ||
|
|
||
| this.router.map(function () { | ||
| this.route('parent', function () { | ||
| this.route('child'); | ||
| this.route('sibling'); | ||
| }); | ||
| }); | ||
|
|
||
| this.add( | ||
| 'route:parent.child', | ||
| Route.extend({ | ||
| afterModel() { | ||
| this.transitionTo('parent.sibling'); | ||
| }, | ||
| }) | ||
| ); | ||
|
|
||
| this.add( | ||
| 'controller:parent', | ||
| Controller.extend({ | ||
| queryParams: ['array', 'string'], | ||
| array: [], | ||
| string: '', | ||
| }) | ||
| ); | ||
|
|
||
| // `/parent/child?array=["one",2]&string=hello` |
There was a problem hiding this comment.
This test case fails with not really helpful error:
Promise rejected during " Calling transitionTo does not serialize multiple times query params already on the activeTransition": Assertion Failed: Cannot call `meta` on stringError message
Which comes from emberA trying to convert a string to array.
emberA(JSON.parse(value as string)`ember.js/packages/@ember/-internals/routing/lib/system/router.ts
Lines 786 to 797 in 20da5b2
But this error is not thrown in an ember application.
Wrapping emberA(JSON.parse(value as string) in a try-catch block results in a readable failing test assertion, like that the expected route qp's are different from the actual ones.
Should we do something to improve error reporting in _deserializeQueryParam? Like, check if JSON.parse(value) actually returns an array?
There was a problem hiding this comment.
Should we do something to improve error reporting in _deserializeQueryParam? Like, check if JSON.parse(value) actually returns an array?
Yes, I think this seems like the best path.
rreckonerr
changed the title
rreckonerr
changed the title
| ['@test Calling transitionTo does not serialize query params already serialized on the activeTransition']( | ||
| assert | ||
| ) { | ||
| assert.expect(3); | ||
|
|
||
| this.router.map(function () { | ||
| this.route('parent', function () { | ||
| this.route('child'); | ||
| this.route('sibling'); | ||
| }); | ||
| }); | ||
|
|
||
| this.add( | ||
| 'route:parent.child', | ||
| Route.extend({ | ||
| afterModel() { | ||
| this.transitionTo('parent.sibling'); | ||
| }, | ||
| }) | ||
| ); | ||
|
|
||
| this.add( | ||
| 'controller:parent', | ||
| Controller.extend({ | ||
| queryParams: ['array', 'string'], | ||
| array: [], | ||
| string: '', | ||
| }) | ||
| ); | ||
|
|
||
| // `/parent/child?array=["one",2]&string=hello` |
There was a problem hiding this comment.
Should we do something to improve error reporting in _deserializeQueryParam? Like, check if JSON.parse(value) actually returns an array?
Yes, I think this seems like the best path.
rwjblue
changed the title
We're using serialized query parameters for active transition instead of deserialized ones. This results in multiple serialization of qp's.
Closes #14174