Skip to content

Commit 9febd38

Browse files
divybotdivybot
authored
fix(ext/url): URLSearchParams Node-compat error messages on invalid this and missing args (#34017)
## Summary Make the failing Node-compat test `test/parallel/test-whatwg-url-custom-searchparams-append.js` pass end-to-end by aligning Deno's `URLSearchParams` and the underlying `webidl` helpers with Node's error contract. The PR landed in two commits — together they cover all six assertions in that test file. ### 1. Branding-check message (initial commit) `webidl.assertBranded` gains an optional `interfaceName` argument. When supplied, the error becomes `Value of "this" must be of type <Iface>` with `code = "ERR_INVALID_THIS"`; otherwise it falls back to `Illegal invocation` (unchanged for non-URLSearchParams interfaces). `URLSearchParams` methods and the pair-iterable mixin (`entries`/`keys`/`values`/`forEach`) pass `"URLSearchParams"` through. ### 2. `ERR_MISSING_ARGS` + symbol message (follow-up commit) - `webidl.requiredArguments` gains an optional `argNames` array. When supplied, it throws a Node-compatible `TypeError` with `code = "ERR_MISSING_ARGS"` and a message of the form `The "name" and "value" arguments must be specified` (Oxford-comma for 3+). Default path unchanged. - `URLSearchParams.{append,delete,get,getAll,has,set}` thread their argument names. - `webidl.converters.DOMString` now throws `TypeError: Cannot convert a Symbol value to a string` (V8's native phrasing — Node uses it too) instead of Deno's previous `"is a symbol, which cannot be converted to a string"`. `String(sym)` does not throw on its own, so the explicit symbol branch is kept; only the message changes. No Deno or WPT test depends on the old phrasing. - `tests/node_compat/config.jsonc` enables `parallel/test-whatwg-url-custom-searchparams-append.js`. All six assertions in the Node test now pass: | # | Call | Expected | |---|------|----------| | 1 | `params.append.call(undefined)` | `TypeError [ERR_INVALID_THIS]: Value of "this" must be of type URLSearchParams` | | 2 | `params.append('a')` | `TypeError [ERR_MISSING_ARGS]: The "name" and "value" arguments must be specified` | | 3-4 | `params.set(obj, ...)` / `params.set(..., obj)` where `obj.toString` throws | `Error: toString` propagates | | 5-6 | `params.set(sym, ...)` / `params.set(..., sym)` | `TypeError: Cannot convert a Symbol value to a string` | ## Test plan - [x] `cargo build --bin deno` - [x] `NODE_TEST_KNOWN_GLOBALS=0 deno run -A test-whatwg-url-custom-searchparams-append.js` exits 0 - [x] `tests/unit/url_search_params_test.ts` — 33 passed - [x] `tests/unit/url_test.ts` — 35 passed - [x] `tests/unit/headers_test.ts` — 28 passed (re-checks DOMString and webidl converters via Headers) - [x] `tools/format.js` clean - [x] `tools/lint.js --js` clean Closes denoland/orchid#52 --------- Co-authored-by: divybot <divybot@users.noreply.github.com>
1 parent 46c1566 commit 9febd38

4 files changed

Lines changed: 62 additions & 30 deletions

File tree

‎ext/web/00_url.js‎

Lines changed: 20 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,11 @@ const { createFilteredInspectProxy } = core.loadExtScript(
4242
const _list = Symbol("list");
4343
const _urlObject = Symbol("url object");
4444

45+
// Pre-frozen argument-name arrays used to produce Node-compatible
46+
// `ERR_MISSING_ARGS` messages from `webidl.requiredArguments`.
47+
const NAME_ARG_NAMES = ["name"];
48+
const APPEND_ARG_NAMES = ["name", "value"];
49+
4550
// WARNING: must match rust code's UrlSetter::*
4651
const SET_HASH = 0;
4752
const SET_HOST = 1;
@@ -170,9 +175,9 @@ class URLSearchParams {
170175
* @param {string} value
171176
*/
172177
append(name, value) {
173-
webidl.assertBranded(this, URLSearchParamsPrototype);
178+
webidl.assertBranded(this, URLSearchParamsPrototype, "URLSearchParams");
174179
const prefix = "Failed to execute 'append' on 'URLSearchParams'";
175-
webidl.requiredArguments(arguments.length, 2, prefix);
180+
webidl.requiredArguments(arguments.length, 2, prefix, APPEND_ARG_NAMES);
176181
name = webidl.converters.USVString(name, prefix, "Argument 1");
177182
value = webidl.converters.USVString(value, prefix, "Argument 2");
178183
ArrayPrototypePush(this[_list], [name, value]);
@@ -184,9 +189,9 @@ class URLSearchParams {
184189
* @param {string} [value]
185190
*/
186191
delete(name, value = undefined) {
187-
webidl.assertBranded(this, URLSearchParamsPrototype);
192+
webidl.assertBranded(this, URLSearchParamsPrototype, "URLSearchParams");
188193
const prefix = "Failed to execute 'append' on 'URLSearchParams'";
189-
webidl.requiredArguments(arguments.length, 1, prefix);
194+
webidl.requiredArguments(arguments.length, 1, prefix, NAME_ARG_NAMES);
190195
name = webidl.converters.USVString(name, prefix, "Argument 1");
191196
const list = this[_list];
192197
let writeIdx = 0;
@@ -216,9 +221,9 @@ class URLSearchParams {
216221
* @returns {string[]}
217222
*/
218223
getAll(name) {
219-
webidl.assertBranded(this, URLSearchParamsPrototype);
224+
webidl.assertBranded(this, URLSearchParamsPrototype, "URLSearchParams");
220225
const prefix = "Failed to execute 'getAll' on 'URLSearchParams'";
221-
webidl.requiredArguments(arguments.length, 1, prefix);
226+
webidl.requiredArguments(arguments.length, 1, prefix, NAME_ARG_NAMES);
222227
name = webidl.converters.USVString(name, prefix, "Argument 1");
223228
const values = [];
224229
const entries = this[_list];
@@ -236,9 +241,9 @@ class URLSearchParams {
236241
* @return {string | null}
237242
*/
238243
get(name) {
239-
webidl.assertBranded(this, URLSearchParamsPrototype);
244+
webidl.assertBranded(this, URLSearchParamsPrototype, "URLSearchParams");
240245
const prefix = "Failed to execute 'get' on 'URLSearchParams'";
241-
webidl.requiredArguments(arguments.length, 1, prefix);
246+
webidl.requiredArguments(arguments.length, 1, prefix, NAME_ARG_NAMES);
242247
name = webidl.converters.USVString(name, prefix, "Argument 1");
243248
const entries = this[_list];
244249
for (let i = 0; i < entries.length; ++i) {
@@ -256,9 +261,9 @@ class URLSearchParams {
256261
* @return {boolean}
257262
*/
258263
has(name, value = undefined) {
259-
webidl.assertBranded(this, URLSearchParamsPrototype);
264+
webidl.assertBranded(this, URLSearchParamsPrototype, "URLSearchParams");
260265
const prefix = "Failed to execute 'has' on 'URLSearchParams'";
261-
webidl.requiredArguments(arguments.length, 1, prefix);
266+
webidl.requiredArguments(arguments.length, 1, prefix, NAME_ARG_NAMES);
262267
name = webidl.converters.USVString(name, prefix, "Argument 1");
263268
if (value !== undefined) {
264269
value = webidl.converters.USVString(value, prefix, "Argument 2");
@@ -275,9 +280,9 @@ class URLSearchParams {
275280
* @param {string} value
276281
*/
277282
set(name, value) {
278-
webidl.assertBranded(this, URLSearchParamsPrototype);
283+
webidl.assertBranded(this, URLSearchParamsPrototype, "URLSearchParams");
279284
const prefix = "Failed to execute 'set' on 'URLSearchParams'";
280-
webidl.requiredArguments(arguments.length, 2, prefix);
285+
webidl.requiredArguments(arguments.length, 2, prefix, APPEND_ARG_NAMES);
281286
name = webidl.converters.USVString(name, prefix, "Argument 1");
282287
value = webidl.converters.USVString(value, prefix, "Argument 2");
283288

@@ -313,7 +318,7 @@ class URLSearchParams {
313318
}
314319

315320
sort() {
316-
webidl.assertBranded(this, URLSearchParamsPrototype);
321+
webidl.assertBranded(this, URLSearchParamsPrototype, "URLSearchParams");
317322
ArrayPrototypeSort(
318323
this[_list],
319324
(a, b) => (a[0] === b[0] ? 0 : a[0] > b[0] ? 1 : -1),
@@ -325,12 +330,12 @@ class URLSearchParams {
325330
* @return {string}
326331
*/
327332
toString() {
328-
webidl.assertBranded(this, URLSearchParamsPrototype);
333+
webidl.assertBranded(this, URLSearchParamsPrototype, "URLSearchParams");
329334
return op_url_stringify_search_params(this[_list]);
330335
}
331336

332337
get size() {
333-
webidl.assertBranded(this, URLSearchParamsPrototype);
338+
webidl.assertBranded(this, URLSearchParamsPrototype, "URLSearchParams");
334339
return this[_list].length;
335340
}
336341
}

‎ext/webidl/00_webidl.js‎

Lines changed: 39 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -407,18 +407,17 @@ converters["unrestricted double?"] = createNullableConverter(
407407
converters["unrestricted double"],
408408
);
409409

410-
converters.DOMString = function (V, prefix, context, opts) {
410+
converters.DOMString = function (V, _prefix, _context, opts) {
411411
if (typeof V === "string") {
412412
return V;
413413
} else if (V === null && opts && opts.treatNullAsEmptyString) {
414414
return "";
415415
} else if (typeof V === "symbol") {
416-
throw makeException(
417-
TypeError,
418-
"is a symbol, which cannot be converted to a string",
419-
prefix,
420-
context,
421-
);
416+
// V8's `String(sym)` returns the symbol description rather than throwing,
417+
// so we throw explicitly to match Node and other WHATWG-conformant
418+
// runtimes, which use V8's native "Cannot convert a Symbol value to a
419+
// string" message (raised by ToPrimitive on Symbols).
420+
throw new TypeError("Cannot convert a Symbol value to a string");
422421
}
423422

424423
return String(V);
@@ -717,8 +716,31 @@ converters["sequence<DOMString>"] = createSequenceConverter(
717716
converters.DOMString,
718717
);
719718

720-
function requiredArguments(length, required, prefix) {
719+
function requiredArguments(length, required, prefix, argNames) {
721720
if (length < required) {
721+
if (argNames !== undefined) {
722+
// Node-compatible error: ERR_MISSING_ARGS with a message that names the
723+
// required arguments, e.g. `The "name" and "value" arguments must be
724+
// specified`.
725+
let formatted;
726+
const n = argNames.length;
727+
if (n === 1) {
728+
formatted = `"${argNames[0]}"`;
729+
} else if (n === 2) {
730+
formatted = `"${argNames[0]}" and "${argNames[1]}"`;
731+
} else {
732+
let joined = "";
733+
for (let i = 0; i < n - 1; i++) {
734+
joined += `"${argNames[i]}", `;
735+
}
736+
formatted = `${joined}and "${argNames[n - 1]}"`;
737+
}
738+
const err = new TypeError(
739+
`The ${formatted} argument${n === 1 ? "" : "s"} must be specified`,
740+
);
741+
err.code = "ERR_MISSING_ARGS";
742+
throw err;
743+
}
722744
const errMsg = `${prefix ? prefix + ": " : ""}${required} argument${
723745
required === 1 ? "" : "s"
724746
} required, but only ${length} present`;
@@ -1167,11 +1189,14 @@ function createBranded(Type) {
11671189
return t;
11681190
}
11691191

1170-
function assertBranded(self, prototype) {
1192+
function assertBranded(self, prototype, interfaceName) {
11711193
if (
11721194
!ObjectPrototypeIsPrototypeOf(prototype, self) || self[brand] !== brand
11731195
) {
1174-
const err = new TypeError("Illegal invocation");
1196+
const message = interfaceName === undefined
1197+
? "Illegal invocation"
1198+
: `Value of "this" must be of type ${interfaceName}`;
1199+
const err = new TypeError(message);
11751200
err.code = "ERR_INVALID_THIS";
11761201
throw err;
11771202
}
@@ -1248,7 +1273,7 @@ function mixinPairIterable(name, prototype, dataSymbol, keyKey, valueKey) {
12481273
}
12491274

12501275
function entries() {
1251-
assertBranded(this, prototype.prototype);
1276+
assertBranded(this, prototype.prototype, name);
12521277
return createDefaultIterator(this, "key+value");
12531278
}
12541279

@@ -1267,7 +1292,7 @@ function mixinPairIterable(name, prototype, dataSymbol, keyKey, valueKey) {
12671292
},
12681293
keys: {
12691294
value: function keys() {
1270-
assertBranded(this, prototype.prototype);
1295+
assertBranded(this, prototype.prototype, name);
12711296
return createDefaultIterator(this, "key");
12721297
},
12731298
writable: true,
@@ -1276,7 +1301,7 @@ function mixinPairIterable(name, prototype, dataSymbol, keyKey, valueKey) {
12761301
},
12771302
values: {
12781303
value: function values() {
1279-
assertBranded(this, prototype.prototype);
1304+
assertBranded(this, prototype.prototype, name);
12801305
return createDefaultIterator(this, "value");
12811306
},
12821307
writable: true,
@@ -1285,7 +1310,7 @@ function mixinPairIterable(name, prototype, dataSymbol, keyKey, valueKey) {
12851310
},
12861311
forEach: {
12871312
value: function forEach(idlCallback, thisArg = undefined) {
1288-
assertBranded(this, prototype.prototype);
1313+
assertBranded(this, prototype.prototype, name);
12891314
const prefix = `Failed to execute 'forEach' on '${name}'`;
12901315
requiredArguments(arguments.length, 1, { prefix });
12911316
idlCallback = converters["Function"](idlCallback, {

‎ext/webidl/internal.d.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,7 @@ declare module "ext:deno_webidl/00_webidl.js" {
370370
length: number,
371371
required: number,
372372
prefix: string,
373+
argNames?: readonly string[],
373374
): void;
374375
type Dictionary = DictionaryMember[];
375376
interface DictionaryMember {
@@ -517,7 +518,7 @@ declare module "ext:deno_webidl/00_webidl.js" {
517518
/**
518519
* Assert that self is branded.
519520
*/
520-
function assertBranded(self: any, type: any): void;
521+
function assertBranded(self: any, type: any, interfaceName?: string): void;
521522

522523
/**
523524
* Create a converter for interfaces.

‎tests/node_compat/config.jsonc‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3752,6 +3752,7 @@
37523752
"parallel/test-whatwg-url-custom-href-side-effect.js": {},
37533753
"parallel/test-whatwg-url-custom-inspect.js": {},
37543754
"parallel/test-whatwg-url-custom-parsing.js": {},
3755+
"parallel/test-whatwg-url-custom-searchparams-append.js": {},
37553756
"parallel/test-whatwg-url-custom-searchparams-sort.js": {},
37563757
"parallel/test-whatwg-url-custom-setters.js": {},
37573758
"parallel/test-whatwg-url-custom-tostringtag.js": {},

0 commit comments

Comments
 (0)