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

url: export URLSearchParams #10801

Closed
wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Member

Fixes: #10761

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

url

@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
@thefourtheye
Copy link
Contributor

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

@TimothyGu
Copy link
Member Author

TimothyGu commented Jan 14, 2017

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

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

@thefourtheye
Copy link
Contributor

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

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

@TimothyGu
Copy link
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.

@joyeecheung
Copy link
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)

@jasnell
Copy link
Member

jasnell commented Jan 15, 2017

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!

@thefourtheye
Copy link
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.

@joyeecheung
Copy link
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?

@joyeecheung joyeecheung added the blocked PRs that are blocked by other issues or PRs. label Jan 15, 2017
@jasnell
Copy link
Member

jasnell commented Jan 15, 2017

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.

@italoacasas italoacasas removed the blocked PRs that are blocked by other issues or PRs. label Jan 19, 2017
@targos
Copy link
Member

targos commented Jan 22, 2017

@targos
Copy link
Member

targos commented Jan 23, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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
Development

Successfully merging this pull request may close these issues.

7 participants