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

Sort URLSearchParams prior to stringification #26

Closed
jeffposnick opened this issue Jun 3, 2015 · 21 comments
Closed

Sort URLSearchParams prior to stringification #26

jeffposnick opened this issue Jun 3, 2015 · 21 comments

Comments

@jeffposnick
Copy link

Inspired by this tweet, I'd argue that the specified behavior for the URLSearchParams stringifier should be changed to either automatically sort by default (unlikely, due to @annevk's objections), or alternatively to support some sort of boolean opt-in to enable sorting.

I'd imagine that sorting based on parameter names, falling back to parameter values in the case of repeated parameters, would make the most sense.

@kornelski
Copy link

I suggest allowing constructor to take an Object and set names/values from object's sorted properties.

I think this would be a nice and convenient opt-in.

@domenic
Copy link
Member

domenic commented Jun 3, 2015

Since URLSearchParams with a different order is actually different semantically, it would make more sense to have something like usp.sorted() that returns a new URLSearchParams object with the order re-shuffled.

@annevk
Copy link
Member

annevk commented Aug 25, 2015

@domenic should that be similar to Array.prototype.sort()? If we add it here I guess we'd want it for FormData and Headers too? And Map objects? Not sure if this really needs to be standard library yet since it's quite trivial to write I think (have not attempted).

@domenic
Copy link
Member

domenic commented Aug 25, 2015

Hmm I'm not sure. If you did want to allow a custom comparator, I'd imagine something like this?

const newUsp = usp.sorted(([k1, v1], [k2, v2]) => v1.localeCompare(v2)); // sort by values

You could start with just a no-arg usp.sorted() that returns them sorted by key, then by value.

@domenic
Copy link
Member

domenic commented Aug 25, 2015

If we add it here I guess we'd want it for FormData and Headers too?

Yeah probably.

And Map objects?

Less clear to me, as there's no good default unless you just assume all keys can be converted to strings.

Not sure if this really needs to be standard library yet since it's quite trivial to write I think (have not attempted).

Yeah, it would be good to see some library code with at least moderate use before we try to bake this in...

@annevk
Copy link
Member

annevk commented Dec 19, 2016

So looking at this somewhat fresh and rereading the tweet thread it seems this is actually specific to URLSearchParams since it affects cache interactions. Neither Headers nor FormData encounter such a scenario. It also seems that a simple lexical sort should be sufficient here and that you actually do want to affect the current instance.

@igrigorik does that sound about right?

If that is correct, just adding a sort() member which uses lexical sort to sort the existing items in the list (using name as the sort key) seems like the way to go.

@annevk
Copy link
Member

annevk commented Jan 10, 2017

@igrigorik @jeffposnick ping.

@jeffposnick
Copy link
Author

👍 to the suggestion at #26 (comment)

@jasnell
Copy link
Collaborator

jasnell commented Jan 10, 2017

If this were to happen, having the new sort() operate the same basic way as Array.prototype.sort() would be ideal, I think. Sort in place with the default sorting on the Unicode codepoint of the keys without value sorting, but allowing a compareFunction to be passed in. The function should sort in place and not be stable.

The reason for the default sort not affecting the value order is that ordering of the values can be significant. We should not assume that it's ok to move the order of the values around.

@igrigorik
Copy link
Member

@annevk yep, another 👍 for #26 (comment).

@annevk
Copy link
Member

annevk commented Jan 12, 2017

@jasnell I was hoping to go with the minimum viable solution here. If you need a callback, you might as well write a quick forEach() or some such. Only sorting keys seems reasonable though.

@jasnell
Copy link
Collaborator

jasnell commented Jan 12, 2017

I can live with that

annevk added a commit to web-platform-tests/wpt that referenced this issue Jan 12, 2017
annevk added a commit that referenced this issue Jan 12, 2017
This method is added to increase cache hits when making requests. It’s
opt-in as the order of code points in a URL’s query is significant by
default. It’s up to applications to decide if name order is not
significant for them.

Tests: web-platform-tests/wpt#4531.

Fixes #26.
@kornelski
Copy link

Please make sure the sort is stable, so that the pseudo-array format foo[]=1&foo[]=2&foo[]=3 preserves order of items in the "array".

@annevk
Copy link
Member

annevk commented Jan 12, 2017

@pornel are you saying that the proposed language fails to capture that? Also, if you have good tests, they'd be appreciated.

@jasnell
Copy link
Collaborator

jasnell commented Jan 12, 2017

I just realized that in my first comment I'd said the the sort need not be stable, that was a typo. I meant that it should. The language in the pr looks good to me.

@kornelski
Copy link

Ah, the PR is fine.

@domenic
Copy link
Member

domenic commented Jan 12, 2017

How do people feel about my suggestion of a .sorted() that returns a new instance, instead of a .sort() that mutates the URLSearchParams instance? I guess maybe .sort() is more what JavaScript programmers expect. You can do a non-destructive sort via

const sorted = (new URLSearchParams(usp)).sort();

I suppose.

@annevk
Copy link
Member

annevk commented Jan 12, 2017

@domenic I think one big problem with that is that it doesn't work well if you got to the URLSearchParams object through new URL(). It wouldn't be reflected back to the URL (other than through setting .search to the stringification).

@domenic
Copy link
Member

domenic commented Jan 12, 2017

Yeah, I guess that makes sense; I forgot there wasn't a searchParams setter.

@domenic
Copy link
Member

domenic commented Jan 12, 2017

Maybe just include an example in the spec of how to do a non-destructive sort like my above.

annevk added a commit to web-platform-tests/wpt that referenced this issue Jan 18, 2017
annevk added a commit that referenced this issue Jan 18, 2017
This method is added to increase cache hits when making requests. It’s
opt-in as the order of code points in a URL’s query is significant by
default. It’s up to applications to decide if name order is not
significant for them.

Tests: web-platform-tests/wpt#4531.

Fixes #26.
@annevk
Copy link
Member

annevk commented Jan 18, 2017

So close to all ending in 4.

TimothyGu added a commit to TimothyGu/node that referenced this issue Feb 11, 2017
TimothyGu added a commit to nodejs/node that referenced this issue Feb 14, 2017
PR-URL: #11098
Fixes: #10760
Ref: whatwg/url#26
Ref: whatwg/url#199
Ref: web-platform-tests/wpt#4531
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
TimothyGu added a commit to nodejs/node that referenced this issue Feb 14, 2017
PR-URL: #11098
Fixes: #10760
Ref: whatwg/url#26
Ref: whatwg/url#199
Ref: web-platform-tests/wpt#4531
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
TimothyGu added a commit to TimothyGu/node that referenced this issue Feb 18, 2017
italoacasas pushed a commit to italoacasas/node that referenced this issue Feb 20, 2017
italoacasas pushed a commit to nodejs/node that referenced this issue Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

6 participants