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: fix setting url.search to the empty string #11105

Closed
wants to merge 3 commits into from

Conversation

TimothyGu
Copy link
Member

Fixes: #11101
Fixes: 98bb65f "url: improving URLSearchParams"

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

url

Fixes: nodejs#11101
Fixes: 98bb65f "url: improving URLSearchParams"
@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Feb 1, 2017
@TimothyGu
Copy link
Member Author

@@ -487,7 +487,7 @@ Object.defineProperties(URL.prototype, {
if (!search) {
ctx.query = null;
ctx.flags &= ~binding.URL_FLAGS_HAS_QUERY;
this[searchParams][searchParams] = {};
this[searchParams][searchParams] = [];
Copy link
Member

Choose a reason for hiding this comment

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

Another way to do it is to not return here, create an else branch to wrap the stuff below and leave the initialization to initSearchParams(this[searchParams], search) later. That way URL doesn't need to know about the data sturcture of URLSearchParams

Copy link
Member Author

Choose a reason for hiding this comment

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

@joyeecheung, done, PTAL.

This does bring a drop to performance when !search. To counter this problem I added a few conditions to avoid parsing as much as possible.

After those changes, when alternative is changed to '', there is a 2.8% drop in performance; but when it is '?' there is a 400% increase, as measured by

node benchmark/compare.js --new ./after --old ./orig \
  --set n=1e5 --set prop=search --set 'url=http://example.com/' \
  --filter properties url

That's pretty good IMO.

@stevenvachon
Copy link

Will this make it to a 7.5.1 or will it have to wait for 7.6?

@jasnell
Copy link
Member

jasnell commented Feb 1, 2017

Unsure. It depends on when 7.5.1 goes out. This won't land until Friday following our typical 48 hour cycle

@@ -614,6 +611,10 @@ function update(url, params) {
// Reused by the URL parse function invoked by
// the href setter, and the URLSearchParams constructor
Copy link
Member

Choose a reason for hiding this comment

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

We can probably just get rid of the comment now, or mention search in it.

@TimothyGu TimothyGu added semver-minor PRs that contain new features and should be released in the next minor version. and removed semver-minor PRs that contain new features and should be released in the next minor version. labels Feb 2, 2017
@TimothyGu
Copy link
Member Author

Landed in 0792d45.

@TimothyGu TimothyGu closed this Feb 3, 2017
TimothyGu added a commit that referenced this pull request Feb 3, 2017
PR-URL: #11105
Fixes: #11101
Fixes: 98bb65f "url: improving URLSearchParams"
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
@TimothyGu TimothyGu deleted the gh-11101 branch February 3, 2017 21:09
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 4, 2017
PR-URL: nodejs#11105
Fixes: nodejs#11101
Fixes: 98bb65f "url: improving URLSearchParams"
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Feb 14, 2017
PR-URL: nodejs#11105
Fixes: nodejs#11101
Fixes: 98bb65f "url: improving URLSearchParams"
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
krydos pushed a commit to krydos/node that referenced this pull request Feb 25, 2017
PR-URL: nodejs#11105
Fixes: nodejs#11101
Fixes: 98bb65f "url: improving URLSearchParams"
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
whatwg-url Issues and PRs related to the WHATWG URL implementation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

this[searchParams].push is not a function
6 participants