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

Fix deserialization of array query params #17151

Closed
wants to merge 1 commit into from
Closed

Fix deserialization of array query params #17151

wants to merge 1 commit into from

Conversation

tchan
Copy link

@tchan tchan commented Oct 25, 2018

When Ember serializes an Array type param, it does so with JSON.stringify.
The deserialize converts back to an Array, but if you have a slow connection, the serialize method gets called again before deserialize which results in re-stringifying a string.

Visual summary of what happens under the hood
screenshot

I've reproduced this behaviour with a twiddle here https://ember-twiddle.com/bc34a1cdf99ce9a013ace8bca3d25162?openFiles=controllers.application.js%2C

This is not ideal but for the above twiddle to work you need to use the allow-control-allow-origin* chrome extension as I'm making an external ajax request each time the query params is updated. Otherwise you'll get a console error No 'Access-Control-Allow-Origin' header is present on the requested resource. Origin 'null' is therefore not allowed access.. Not sure how to bypass this otherwise.

Steps:

  1. Simulate a slow connection by applying network throttling. In the gif I used a custom profile on Chrome with 10kb/s and 20,000ms of added latency.
  2. Click a checkbox (arrays query param)
  3. Start typing in the search bar.

Gif of what happens to the URL due to deserializing getting called before serialize which is apparent with a really slow internet connection
emberarrays

Gif of what happens with a standard connection (expected behaviour)
normalconnection

Related issues:
#14174
#13591
#14171

@acorncom
Copy link
Contributor

@tchan think you could add a test here? This is likely to be a future area of churn so might help to have your case covered so it doesn’t come up again ...

@tpetrone
Copy link

Greetings, any updates on this?
Tks

@tchan
Copy link
Author

tchan commented Nov 27, 2018

Hey @tpetrone , I haven't had any time to write a test for this, feel free to take over if you want :)

@tpetrone
Copy link

Tks @tchan !

@arthurmde do you think we can tackle it?

@tchan tchan closed this Feb 22, 2019
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