Skip to content
This repository has been archived by the owner on Dec 14, 2021. It is now read-only.

Add URLSearchParams.prototype.sort() #135

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

hzr
Copy link
Contributor

@hzr hzr commented Sep 19, 2017

@@ -151,6 +151,15 @@ QUnit.test('Parameter Mutation', function(assert) {
url.searchParams.set('b', '3');
assert.deepEqual(url.searchParams.getAll('b'), ['3']);
assert.equal(url.href, 'http://example.com/?a=1&b=3&c=1');

url.href = 'http://example.com?c=1&a=2&a=4&b=3';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if these tests belong in this section.

@inexorabletash
Copy link
Owner

The problem with the patch as-is is that several browser implementations shipped a version without sort(). That means very old browsers would use the full polyfill with sort(), and very new browsers don't need the polyfill. But in between there are browsers that would have URLSearchParams but no sort() (such as the current versions of Chrome)

To add this safely it'll be necessary to include a polyfill for a native URLSearchParams without sort().

@hzr
Copy link
Contributor Author

hzr commented Sep 21, 2017

Yes, good point. I can look into this when I have the time.

@inexorabletash
Copy link
Owner

Thanks! I greatly appreciate the PR submission and especially the inclusion of tests.

@inexorabletash
Copy link
Owner

Also, obviously, CI running on PRs would catch this, but I haven't been spending any time on this project lately. :(

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

Successfully merging this pull request may close these issues.

None yet

2 participants