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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
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
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
@targos
Copy link
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
Member Author

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

@targos
Copy link
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 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
Member Author

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

@TimothyGu
Copy link
Member Author

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

@TimothyGu
Copy link
Member Author

@jasnell @joyeecheung mind taking a look?

@TimothyGu
Copy link
Member Author

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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
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 <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@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

@targos targos targos approved these changes

@jasnell jasnell jasnell approved these changes

@refack refack refack approved these changes

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