url: fix setting `url.search` to the empty string #11105

Closed
wants to merge 3 commits into
from

Conversation

Projects
6 participants
@TimothyGu
Member

TimothyGu commented Feb 1, 2017

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

url: fix setting `url.search` to the empty string
Fixes: #11101
Fixes: 98bb65f "url: improving URLSearchParams"
@jasnell

jasnell approved these changes Feb 1, 2017

@targos

targos approved these changes Feb 1, 2017

lib/internal/url.js
@@ -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] = [];

This comment has been minimized.

@joyeecheung

joyeecheung Feb 1, 2017

Member

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

@joyeecheung

joyeecheung Feb 1, 2017

Member

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

This comment has been minimized.

@TimothyGu

TimothyGu Feb 1, 2017

Member

@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.

@TimothyGu

TimothyGu Feb 1, 2017

Member

@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

This comment has been minimized.

Show comment
Hide comment
@stevenvachon

stevenvachon Feb 1, 2017

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

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

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 1, 2017

Member

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

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

lib/internal/url.js
@@ -614,6 +611,10 @@ function update(url, params) {
// Reused by the URL parse function invoked by
// the href setter, and the URLSearchParams constructor

This comment has been minimized.

@joyeecheung

joyeecheung Feb 2, 2017

Member

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

@joyeecheung

joyeecheung Feb 2, 2017

Member

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

@TimothyGu TimothyGu added this to Spec Irrelavent / De-facto in WHATWG URL implementation Feb 2, 2017

@TimothyGu TimothyGu added semver-minor and removed semver-minor labels Feb 2, 2017

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Feb 3, 2017

Member

Landed in 0792d45.

Member

TimothyGu commented Feb 3, 2017

Landed in 0792d45.

@TimothyGu TimothyGu closed this Feb 3, 2017

TimothyGu added a commit that referenced this pull request Feb 3, 2017

url: fix setting `url.search` to the empty string
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 TimothyGu:gh-11101 branch Feb 3, 2017

@TimothyGu TimothyGu moved this from Spec Irrelavent / De-facto to Done in WHATWG URL implementation Feb 3, 2017

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 4, 2017

url: fix setting `url.search` to the empty string
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 added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

url: fix setting `url.search` to the empty string
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 added a commit to KryDos/node that referenced this pull request Feb 25, 2017

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