-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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: make URLSearchParams properties spec-compliant #11057
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with green CI
@joyeecheung, just requested a review from you as you are the author of #10408 |
@@ -647,6 +647,35 @@ function getObjectFromParams(array) { | |||
return obj; | |||
} | |||
|
|||
// Mainly to mitigate func-name-matching ESLint rule | |||
function defineIDLClass(proto, classStr, obj) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if there is a possibility/necessity to generate these stuff from an IDL file, like what browsers do, if there will be more convergence with Web APIs in Node core (console, crypto, streams, etc.) Not that this is what this PR should deal with though(and this is just to mitigate func-name-matching anyway, not full-on generation), just a thought.
Also the name is a little bit too general, could be something like defineIDLData
or defineIDLValues
, since it's about properties that need to be defined with value
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would likely be a bit overkill at the moment. This is the only IDL defined API we have currently. Eventually if we go down that road more then we can consider this.
value: obj[key] | ||
}); | ||
} | ||
for (const key of Object.getOwnPropertySymbols(obj)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be easier to read if s/key/symbol/, or just separate the two mixins(take operations
and symbols
as arguments instead of one obj
)
Landed in 90c2ac7 |
Define
URLSearchParams
methods as enumerable, writable, configurable, as have already been done toURL
in #10408. Also adjust the@@toStringTag
variable, as done toURL
in #10906.Fixes: #10799
Benchmark (no significant differences)
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
url