-
Notifications
You must be signed in to change notification settings - Fork 141
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
Simplify constructing URLSearchParams #175
Conversation
This would certainly be interesting and I would implement this in the Node.js impl if adopted in the spec. I'd like to clarify: the |
Yeah. Though the USVString-ness happens through coercion, so if you have numbers there that would technically work through the magic of IDL. What's blocking this change is tests and asking browsers if they'd ship this. |
Understood. I saw the "needs implementer interest" label. Wanted to express that we would impl and ship in Node.js if added and I think it's a worthwhile change so +1 from this side. I've opened an issue to track the progress on this and will keep an eye on it. |
The behavior here is governed by the definiton for how to convert a JS value into a union type. I think it ends up looking basically like this: if (arg === null || arg === undefined) {
// record<USVString, USVString>
// add zero key/value pairs to the URLSearchParams
} else if (typeof arg === "object") {
const iterator = arg[Symbol.iterator];
if (iterator === undefined || iterator === null) {
// record<USVString, USVString>
const map = new Map();
for (const key of Object.getOwnPropertyNames(arg)) {
const desc = Object.getOwnPropertyDescriptor(arg, key);
if (desc !== undefined && desc.enumerable) {
const typedKey = toUSVString(key);
const typedValue = toUSVString(arg[key]);
map.set(typedKey, typedValue);
}
}
// Use `map` to add key/value pairs to the URLSearchParams
} else {
// sequence<sequence<USVString>>
const pairs = [];
// Note: per spec we have to first exhaust the lists (convert
// from JS value to sequence<sequence<USVString>>) then process them
for (const x of iterator) {
const gotten = [];
pairs.push(gotten);
for (const y of x) {
gotten.push(toUSVString(y));
}
}
for (const pair of pairs) {
if (pair.length !== 2) {
throw new TypeError();
}
// Add (`pair[0]`, `pair[1]`) key/value pair to URLSearchParams
}
}
} else {
// USVString
const string = toUSVString(arg);
// parse and process the string to add key/value pairs to the URLSearchParams
} |
(To be clear, it's certainly very cool that Node.js ships a compatible URL parser and API these days and your support hopefully makes for a more compelling case across the board. I didn't mean to sound dismissive.) |
Mozilla supports this change. @mikewest, I take it Chrome would implement this too in due course? |
Once the tests are reviewed and landed I'll file browser issues and land this change. |
Yeah. This seems totally reasonable to me. Added some flags to the bug you filed against Chromium. Thanks! |
PR-URL: #11060 Fixes: #10635 Ref: whatwg/url#175 Ref: web-platform-tests/wpt#4523 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #11060 Ref: whatwg/url#175 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#11060 Fixes: nodejs#10635 Ref: whatwg/url#175 Ref: web-platform-tests/wpt#4523 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: nodejs#11060 Ref: whatwg/url#175 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Follow up recent spec addition[1,2] and support sequence<sequence<USVString>> initializers for URLSearchParams. 1 - whatwg/url#27 2 - whatwg/url#175 R=tyoshino,haraken BUG=680531 Review-Url: https://codereview.chromium.org/2725593003 Cr-Commit-Position: refs/heads/master@{#453903}
PR-URL: nodejs#12507 Ref: whatwg/url#175 Reviewed-By: James M Snell <jasnell@gmail.com>
PR-URL: #12507 Ref: whatwg/url#175 Reviewed-By: James M Snell <jasnell@gmail.com>
Adapt to whatwg/url#175, which dropped URLSearchParams itself from the union accepted by the constructor. The constructor now takes an optional (sequence<sequence<USVString>> or record<USVString, USVString> or USVString) instead. Existing uses of `new URLSearchParams(<existing URLSearchParam>)` continue to work, as per WebIDL these objects are converted to sequences because URLSearchParams is iterable. Intent to ship/implement thread: https://groups.google.com/a/chromium.org/d/msg/blink-dev/AGyufsf5XU4/CBeVLmT7BgAJ Bug: 680531, 697378 Change-Id: Ie82d1151acc4334628be0cc258bc20e5a0dc1d15 Reviewed-on: https://chromium-review.googlesource.com/544919 Commit-Queue: Raphael Kubo da Costa (rakuco) <raphael.kubo.da.costa@intel.com> Reviewed-by: Kentaro Hara <haraken@chromium.org> Reviewed-by: Mike West <mkwst@chromium.org> Cr-Commit-Position: refs/heads/master@{#482588}
Fixes #27.