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

Add URLSearchParams.prototype.sort() #199

Merged
merged 6 commits into from
Jan 18, 2017
Merged

Conversation

annevk
Copy link
Member

@annevk annevk commented 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.

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 Author

annevk commented Jan 12, 2017

@tabatkins @plinss https://api.csswg.org/bikeshed/ returns 400s to Travis. Something going on?

@annevk
Copy link
Member Author

annevk commented Jan 12, 2017

Apologies for having pinged you. It was my error.

@annevk
Copy link
Member Author

annevk commented Jan 12, 2017

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
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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").

Copy link
Collaborator

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.

Copy link
Member Author

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.

Copy link
Member

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.

@annevk
Copy link
Member Author

annevk commented Jan 13, 2017

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>
Copy link
Member

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.

Copy link
Member Author

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,
Copy link
Member

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, should have asked.

@annevk
Copy link
Member Author

annevk commented Jan 17, 2017

@TimothyGu all good now?

@aphillips
Copy link

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.

@annevk
Copy link
Member Author

annevk commented Jan 17, 2017

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.

@annevk annevk merged commit 960f607 into master Jan 18, 2017
@annevk annevk deleted the annevk/URLSearchParams-sort branch January 18, 2017 07:59
TimothyGu added a commit to TimothyGu/node that referenced this pull request Feb 11, 2017
TimothyGu added a commit to nodejs/node that referenced this pull request 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 pull request 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 pull request Feb 18, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 20, 2017
italoacasas pushed a commit to nodejs/node that referenced this pull request Feb 22, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants