Skip to content

url: update sort() behavior with no params#14185

Closed
TimothyGu wants to merge 1 commit intonodejs:masterfrom
TimothyGu:url-searchparams-update
Closed

url: update sort() behavior with no params#14185
TimothyGu wants to merge 1 commit intonodejs:masterfrom
TimothyGu:url-searchparams-update

Conversation

@TimothyGu
Copy link
Copy Markdown
Member

@TimothyGu TimothyGu commented Jul 12, 2017 •
edited
Loading

This reverts commit b465cd0.

Refs: web-platform-tests/wpt#6445
Fixes: #14020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@TimothyGu TimothyGu requested review from jasnell and targos July 12, 2017 06:16
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jul 12, 2017
@mscdex
Copy link
Copy Markdown
Contributor

mscdex commented Jul 12, 2017 •
edited
Loading

I think the commit message should be the standard/default revert message (as far as the non-metadata text goes).

@TimothyGu TimothyGu changed the title url: revert "fix URL query update if searchParams changes" Revert "url: fix URL query update if searchParams changes" Jul 12, 2017
@TimothyGu TimothyGu force-pushed the url-searchparams-update branch from 66feb3f to e6ada88 Compare July 12, 2017 06:25
@targos
Copy link
Copy Markdown
Member

targos commented Jul 12, 2017

Is there any active discussion about changing the spec, knowing that Chrome, Firefox and Node are currently doing the same thing? I found whatwg/url#332 which has no answer.

@TimothyGu TimothyGu added the wip Issues and PRs that are still a work in progress. label Jul 12, 2017
@TimothyGu
Copy link
Copy Markdown
Member Author

TimothyGu commented Jul 12, 2017

@targos I'll put this on hold until whatwg/url#332 is resolved.

@targos
Copy link
Copy Markdown
Member

targos commented Jul 18, 2017

Now that whatwg/url#336 has been merged, this is not needed anymore.
I think that now we are almost spec-compliant. Calling url.searchParams.sort() with empty searchParams should remove the trailing ? but it does not in Node (it's taking a shortcut here).

@TimothyGu TimothyGu force-pushed the url-searchparams-update branch from e6ada88 to bb48b2b Compare July 24, 2017 04:52
@TimothyGu TimothyGu removed the wip Issues and PRs that are still a work in progress. label Jul 24, 2017
@TimothyGu TimothyGu changed the title Revert "url: fix URL query update if searchParams changes" url: update sort() behavior with no params Jul 24, 2017
@TimothyGu
Copy link
Copy Markdown
Member Author

TimothyGu commented Jul 24, 2017

I've rebased this on top of master and it now implements the revised spec. Tests are added accordingly.

@TimothyGu
Copy link
Copy Markdown
Member Author

TimothyGu commented Jul 24, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9315/

@TimothyGu
Copy link
Copy Markdown
Member Author

TimothyGu commented Jul 30, 2017

@jasnell @joyeecheung mind taking a look?

@TimothyGu
Copy link
Copy Markdown
Member Author

TimothyGu commented Aug 1, 2017

Landed in 57630ad.

@TimothyGu TimothyGu closed this Aug 1, 2017
TimothyGu added a commit that referenced this pull request Aug 1, 2017
@TimothyGu
url: update sort() behavior with no params
57630ad 
PR-URL: #14185
Refs: whatwg/url#336
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Aug 1, 2017
@TimothyGu @addaleax
url: update sort() behavior with no params
3225942 
PR-URL: #14185
Refs: whatwg/url#336
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@addaleax addaleax mentioned this pull request Aug 2, 2017
Merged
@TimothyGu TimothyGu deleted the url-searchparams-update branch July 29, 2018 23:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@targos targos targos approved these changes

+1 more reviewer

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

whatwg-url Issues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

URL#searchParams#delete removes "?"

6 participants

@TimothyGu @mscdex @targos @refack @jasnell @nodejs-github-bot