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: more precise URLSearchParams constructor #13026

Closed
wants to merge 1 commit into from

Conversation

TimothyGu
Copy link
Member

Refs: web-platform-tests/wpt#5813

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

@nodejs-github-bot nodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels May 14, 2017
@TimothyGu
Copy link
Member Author

@mscdex
Copy link
Contributor

mscdex commented May 14, 2017

Previously I saw noticeable performance impacts when using a helper function like this, you may want to benchmark it vs. explicitly inlining.

// Returns true if `val` is of the spec Object type (including functions), but
// false if `val` is of the spec Null type.
function isObject(val) {
return (typeof val === 'object' && val !== null) || typeof val === 'function';
Copy link
Member

Choose a reason for hiding this comment

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

If we do want a helper function..this seems to be equivalent to !util.isPrimitive(val)?

key = toUSVString(key);
const value = toUSVString(init[key]);
this[searchParams].push(key, value);
for (const key of Reflect.ownKeys(init)) {
Copy link
Member

Choose a reason for hiding this comment

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

would likely be faster to use a normal for-n loop here

Copy link
Member

Choose a reason for hiding this comment

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

@jasnell Did you mean for-in? I think that one doesn't give you the property symbols.

Copy link
Member

Choose a reason for hiding this comment

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

I mean..

const keys = Reflect.ownKeys(init);
for (var n = 0; n < keys.length; n++) {
  // ...
}

@@ -156,7 +167,8 @@ test(function() {
[
{ "input": {"+": "%C2"}, "output": [["+", "%C2"]], "name": "object with +" },
{ "input": {c: "x", a: "?"}, "output": [["c", "x"], ["a", "?"]], "name": "object with two keys" },
{ "input": [["c", "x"], ["a", "?"]], "output": [["c", "x"], ["a", "?"]], "name": "array with two keys" }
{ "input": [["c", "x"], ["a", "?"]], "output": [["c", "x"], ["a", "?"]], "name": "array with two keys" },
Copy link
Member

Choose a reason for hiding this comment

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

long lines here. does this pass lint?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is from WPT so it's wrapped in eslint-disable

@TimothyGu TimothyGu force-pushed the urlsearchparams-record branch from 66bc147 to 2fefc33 Compare May 21, 2017 02:09
@TimothyGu
Copy link
Member Author

Previously I saw noticeable performance impacts when using a helper function like this

Unfortunately this is true for this PR as well. Now the checks are specifically inlined.

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

@@ -226,6 +246,8 @@ test(() => {
assert.throws(() => new URLSearchParams([['a', obj]]), /^Error: toString$/);
assert.throws(() => new URLSearchParams(sym),
/^TypeError: Cannot convert a Symbol value to a string$/);
assert.throws(() => new URLSearchParams({ [sym]: 'a' }),
/^TypeError: Cannot convert a Symbol value to a string$/);
Copy link
Member

Choose a reason for hiding this comment

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

perhaps create the regex once and reuse it?

@@ -817,7 +817,8 @@ class URLSearchParams {
constructor(init = undefined) {
if (init === null || init === undefined) {
this[searchParams] = [];
} else if (typeof init === 'object') {
} else if (typeof init === 'object' && init !== null ||
typeof init === 'function') {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how others feel, but I prefer to use parentheses when I’m mixing logical and and logical or because I can never remember the precedence rules. ;)

@TimothyGu TimothyGu force-pushed the urlsearchparams-record branch from 2fefc33 to 67d56cc Compare May 31, 2017 23:52
@TimothyGu
Copy link
Member Author

@jasnell & @addaleax: comments fixed and rebased.

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

jasnell pushed a commit that referenced this pull request Jun 1, 2017
PR-URL: #13026
Ref: web-platform-tests/wpt#5813
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell
Copy link
Member

jasnell commented Jun 1, 2017

Landed in 95eef9b

@jasnell jasnell closed this Jun 1, 2017
jasnell pushed a commit that referenced this pull request Jun 5, 2017
PR-URL: #13026
Ref: web-platform-tests/wpt#5813
Reviewed-By: Daijiro Wachi <daijiro.wachi@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@jasnell jasnell mentioned this pull request Jun 5, 2017
@TimothyGu TimothyGu deleted the urlsearchparams-record branch June 19, 2017 07:46
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.

7 participants