GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — url: export URLSearchParams by TimothyGu · Pull Request #10801 · nodejs/node

Conversation

@TimothyGu

@TimothyGu TimothyGu commented Jan 14, 2017

Copy link
Copy Markdown
Member

Fixes: #10761

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

url

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@TimothyGu
url: export URLSearchParams
e439047
Fixes: nodejs#10761
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x url Issues and PRs related to the legacy built-in url module. whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 14, 2017
joyeecheung
joyeecheung approved these changes Jan 14, 2017
View reviewed changes
@TimothyGu TimothyGu mentioned this pull request Jan 14, 2017
url: export URLSearchParams alongside URL #10761
Closed
@thefourtheye

thefourtheye commented Jan 14, 2017

Copy link
Copy Markdown
Contributor

@jasnell Are we compatible with https://url.spec.whatwg.org/#urlsearchparams?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@TimothyGu

TimothyGu commented Jan 14, 2017 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member Author

@thefourtheye, pretty much. The only (minor) things that are yet to be done to accomplish full spec compliance are

  • Enumerability of prototype properties (filed as url: make URLSearchParams properties spec-compliant #10799)
  • Order of serialized URL params in case of duplicates (fixed in More URLSearchParams improvements #10399)
  • Fully spec-compliant serialization (test case)
  • Fully spec-compliant parsing for & (url: Improve WHATWG URLSearchParams spec compliance #9484 (comment))

For the last two bullet points, I have a local branch that fixes them, but have not submitted yet pending this PR and #10399.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@thefourtheye

thefourtheye commented Jan 14, 2017

Copy link
Copy Markdown
Contributor

@TimothyGu That's a great summary :-) Thanks :-)

Can we expose this once we are spec compliant? Also, this needs documentation changes as well.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@TimothyGu

TimothyGu commented Jan 14, 2017

Copy link
Copy Markdown
Member Author

@thefourtheye, the documentation is already written by @jasnell in #10620.

I also just noticed #10635, which means that to bring the current implementation to 100% compliance with the most current spec we'll also need to change a few more things in the constructor.

Let's put a hold on this for the time being then.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented Jan 14, 2017

Copy link
Copy Markdown
Member

@thefourtheye There is a GitHub project tracking the WHATWG URL implementation: https://github.com/nodejs/node/projects/5 (the columns these issues/PRs are placed are shown in the sidebar of issue/PR page, below the labels)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@jasnell

jasnell commented Jan 15, 2017

Copy link
Copy Markdown
Member

btw, @TimothyGu, @joyeecheung and @targos ... I really must thank the three of you for the work that you've each put in on improvinng the URL implementation. I really do appreciate the help!

joyeecheung, thefourtheye, TimothyGu, and targos reacted with heart emoji
All reactions
  • ❤️ 4 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell
jasnell approved these changes Jan 15, 2017
View reviewed changes
@thefourtheye

thefourtheye commented Jan 15, 2017

Copy link
Copy Markdown
Contributor

I am not really against this change. But once we expose this, people might come and say this doesn't work as it does in browsers. Thats why I just want to make sure we are spec complaint before exposing.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung

joyeecheung commented Jan 15, 2017

Copy link
Copy Markdown
Member

@thefourtheye I am +1 about getting URLSearchParams spec compliant first before exposing it. So if I understand correctly this PR is blocked by #10399?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Jan 15, 2017
@joyeecheung joyeecheung mentioned this pull request Jan 15, 2017
More URLSearchParams improvements #10399
Closed
3 tasks done
@jasnell

jasnell commented Jan 15, 2017

Copy link
Copy Markdown
Member

Keep in mind that this code is all still considered Experimental. It's better if it were completely spec compliant but that shouldn't be a blocker. Also keep in mind that the spec evolves quickly. There's always going to be a possibility that bits are out of spec given how the spec itself evolves over time.

TimothyGu reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@TimothyGu TimothyGu mentioned this pull request Jan 15, 2017
url: track status of a spec-compliant URLSearchParams #10821
Closed
11 tasks done
@italoacasas italoacasas removed the blocked PRs that are blocked by other issues or PRs. label Jan 19, 2017
targos
targos approved these changes Jan 22, 2017
View reviewed changes
@targos

targos commented Jan 22, 2017

Copy link
Copy Markdown
Member

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos referenced this pull request Jan 23, 2017
@TimothyGu @targos
url: export URLSearchParams
326e967
Fixes: #10761
@targos

targos commented Jan 23, 2017

Copy link
Copy Markdown
Member

Landed by mistake without metadata in 326e967. Sorry about that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@targos targos closed this Jan 23, 2017
@TimothyGu
TimothyGu deleted the urlsearchparams-export branch January 23, 2017 15:33
@adamlaska adamlaska mentioned this pull request Nov 25, 2023
[Snyk] Fix for 6 vulnerabilities adamlaska/node#112
Open
@aliscco aliscco mentioned this pull request Nov 28, 2023
[Snyk] Security upgrade eslint from 4.19.1 to 5.5.0 aliscco/alisco-node#665
Open
@leonardoadame leonardoadame mentioned this pull request Nov 28, 2023
[Snyk] Fix for 1 vulnerabilities leonardoadame/node#187
Open
This was referenced Nov 28, 2023
[Snyk] Security upgrade eslint from 4.19.1 to 5.5.0 aliscco/alisco-node#691
Open
[Snyk] Fix for 8 vulnerabilities aliscco/alisco-node#698
Open
This was referenced Nov 29, 2023
[Snyk] Security upgrade eslint from 4.19.1 to 5.5.0 leonardoadame/node#212
Open
[Snyk] Security upgrade eslint from 4.19.1 to 5.5.0 leonardoadame/node#213
Open
This was referenced Nov 29, 2023
[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#723
Open
[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#727
Open
[Snyk] Fix for 4 vulnerabilities aliscco/alisco-node#746
Open
@leonardoadame leonardoadame mentioned this pull request Nov 29, 2023
[Snyk] Fix for 4 vulnerabilities leonardoadame/node#239
Open
@aliscco aliscco mentioned this pull request Nov 29, 2023
[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#766
Open
@leonardoadame leonardoadame mentioned this pull request Nov 29, 2023
[Snyk] Fix for 1 vulnerabilities leonardoadame/node#262
Open
This was referenced Nov 30, 2023
[Snyk] Fix for 5 vulnerabilities aliscco/alisco-node#822
Open
[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#830
Open
[Snyk] Fix for 6 vulnerabilities aliscco/alisco-node#835
Open
[Snyk] Fix for 1 vulnerabilities aliscco/alisco-node#840
Open
This was referenced Nov 10, 2024
Missing version info about some members of URL class & URLSearchParams class #55806
Closed
Update Node.js data for URLSearchParams mdn/browser-compat-data#24998
Merged
doc: fix history info for URLSearchParams #55837
Closed
doc: fix history info for URLSearchParams #55840
Closed
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
@joyeecheung joyeecheung joyeecheung approved these changes
+1 more reviewer
@targos targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

url Issues and PRs related to the legacy built-in url module. 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.

Uh oh!

There was an error while loading. Please reload this page.

7 participants

@TimothyGu @thefourtheye @joyeecheung @jasnell @targos @italoacasas @nodejs-github-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.