url: more precise URLSearchParams constructor #13026
Conversation
nodejs-github-bot
added
dont-land-on-v4.x
whatwg-url
|
Previously I saw noticeable performance impacts when using a helper function like this, you may want to benchmark it vs. explicitly inlining. |
lib/internal/url.js
Outdated
There was a problem hiding this comment.
If we do want a helper function..this seems to be equivalent to !util.isPrimitive(val)?
lib/internal/url.js
Outdated
There was a problem hiding this comment.
would likely be faster to use a normal for-n loop here
There was a problem hiding this comment.
@jasnell Did you mean for-in? I think that one doesn't give you the property symbols.
There was a problem hiding this comment.
I mean..
const keys = Reflect.ownKeys(init);
for (var n = 0; n < keys.length; n++) {
// ...
}
There was a problem hiding this comment.
long lines here. does this pass lint?
There was a problem hiding this comment.
This is from WPT so it's wrapped in eslint-disable
TimothyGu
force-pushed
the
urlsearchparams-record
branch
from
66bc147 to
2fefc33
Compare
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/ |
There was a problem hiding this comment.
perhaps create the regex once and reuse it?
lib/internal/url.js
Outdated
There was a problem hiding this comment.
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
force-pushed
the
urlsearchparams-record
branch
from
2fefc33 to
67d56cc
Compare
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>
|
Landed in 95eef9b |
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>
Refs: web-platform-tests/wpt#5813
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
url