-
Notifications
You must be signed in to change notification settings - Fork 141
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
Add URLSearchParams.prototype.sort() #199
Conversation
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.
@tabatkins @plinss https://api.csswg.org/bikeshed/ returns 400s to Travis. Something going on? |
Apologies for having pinged you. It was my error. |
Feedback: @domenic requests an example for non-destructive sort. |
these steps: | ||
|
||
<ol> | ||
<li><p>Lexicographically sort all name-value pairs, if any, by name, while preserving the relative |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I mentioned in the tests PR, do we have a definition for "lexicographically"? I assume that means é sorts after z, not before f, but I can't be sure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't really although we do use it here and there. I wonder if @r12a or @aphillips can help us out. I always assumed it meant code point order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We use the term for the Headers
object too, but that's restricted to bytes (although I guess even there it might be confusing, does "@" come before or after "a").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The MDN docs for Array.prototype.sort explicitly calls out using "Unicode codepoint order". I'd recommend the same wording here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having looked at https://tc39.github.io/ecma262/#sec-abstract-relational-comparison I think we want code unit order, not code point order. So something like:
Sort all name-value pairs, if any, by name on code unit, while preserving the relative order between duplicate names, if any.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good, but breaking that up into a few sentences wouldn't hurt IMO:
Sort all name-value pairs, if any, by their names. Sorting must be done by comparison of code units. The relative order between name-value pairs with equal names must be preserved.
Note to self: file some follow-up issues for other places that use "alphabetical" (HTML), "lexicographical" (Fetch), and look if we have more places that could use clarification. |
{{SearchParams}} object: | ||
|
||
<pre><code class=lang-javascript> | ||
const sorted = (new URLSearchParams(url.search)).sort();</code></pre> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently sort()
returns void, so we either need to make sort
return this
or change this example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I opted for fixing the example. Might change if we fix #90 as requested I suppose.
@@ -2969,6 +3004,7 @@ Tab Atkins, | |||
Tantek Çelik, | |||
Tim Berners-Lee, | |||
簡冠庭 (Tim Guan-tin Chien), | |||
Timothy Gu, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please attribute to me as
Tiancheng "Timothy" Gu
which contains my legal name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, should have asked.
@TimothyGu all good now? |
The new text doesn't define 'code units' and neither does the document directly or by reference. In addition, it doesn't specify why sort of code units are being sorted. Sorting into UTF-8 byte order doesn't seem to be the intention. I suspect that UTF-16 code unit order is expected. Make that explicit? I could argue for code point order, but suspect the overhead isn't worth it. |
The plan is to make sure it's defined in https://infra.spec.whatwg.org/ in due course. whatwg/infra#1 has various ideas and whatwg/infra#55 tracks sorting in particular. I haven't really come up with a concrete proposal yet since in part I feel that I should study more of the various usage patterns we have first. |
Fixes: nodejs#10760 Ref: whatwg/url#26 Ref: whatwg/url#199 Ref: web-platform-tests/wpt#4531
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>
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>
Backport-of: nodejs#11098 Fixes: nodejs#10760 Ref: whatwg/url#26 Ref: whatwg/url#199 Ref: web-platform-tests/wpt#4531
Backport-of: nodejs#11098 Fixes: nodejs#10760 Ref: whatwg/url#26 Ref: whatwg/url#199 Ref: web-platform-tests/wpt#4531
Backport-of: #11098 Fixes: #10760 Ref: whatwg/url#26 Ref: whatwg/url#199 Ref: web-platform-tests/wpt#4531
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.