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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

url: make URLSearchParams properties spec-compliant #11057

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
232903b
url: make URLSearchParams and its Iterator's properties spec-compliant
TimothyGu Jan 24, 2017
d9b9d79
test: extend test-whatwg-url-tostringtag.js
TimothyGu Jan 28, 2017
ee6d211
Fix linting
TimothyGu Jan 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
214 changes: 121 additions & 93 deletions lib/internal/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -647,6 +647,35 @@ function getObjectFromParams(array) {
return obj;
}

// Mainly to mitigate func-name-matching ESLint rule
function defineIDLClass(proto, classStr, obj) {
Copy link
Member

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.

Copy link
Member

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.

// https://heycam.github.io/webidl/#dfn-class-string
Object.defineProperty(proto, Symbol.toStringTag, {
writable: false,
enumerable: false,
configurable: true,
value: classStr
});

// https://heycam.github.io/webidl/#es-operations
for (const key of Object.keys(obj)) {
Object.defineProperty(proto, key, {
writable: true,
enumerable: true,
configurable: true,
value: obj[key]
});
}
for (const key of Object.getOwnPropertySymbols(obj)) {
Copy link
Member

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)

Object.defineProperty(proto, key, {
writable: true,
enumerable: false,
configurable: true,
value: obj[key]
});
}
}

class URLSearchParams {
constructor(init = '') {
if (init instanceof URLSearchParams) {
Expand All @@ -662,11 +691,39 @@ class URLSearchParams {
this[context] = null;
}

get [Symbol.toStringTag]() {
return this instanceof URLSearchParams ?
'URLSearchParams' : 'URLSearchParamsPrototype';
[util.inspect.custom](recurseTimes, ctx) {
if (!this || !(this instanceof URLSearchParams)) {
throw new TypeError('Value of `this` is not a URLSearchParams');
}

const separator = ', ';
const innerOpts = Object.assign({}, ctx);
if (recurseTimes !== null) {
innerOpts.depth = recurseTimes - 1;
}
const innerInspect = (v) => util.inspect(v, innerOpts);

const list = this[searchParams];
const output = [];
for (var i = 0; i < list.length; i += 2)
output.push(`${innerInspect(list[i])} => ${innerInspect(list[i + 1])}`);

const colorRe = /\u001b\[\d\d?m/g;
const length = output.reduce(
(prev, cur) => prev + cur.replace(colorRe, '').length + separator.length,
-separator.length
);
if (length > ctx.breakLength) {
return `${this.constructor.name} {\n ${output.join(',\n ')} }`;
} else if (output.length) {
return `${this.constructor.name} { ${output.join(separator)} }`;
} else {
return `${this.constructor.name} {}`;
}
}
}

defineIDLClass(URLSearchParams.prototype, 'URLSearchParams', {
append(name, value) {
if (!this || !(this instanceof URLSearchParams)) {
throw new TypeError('Value of `this` is not a URLSearchParams');
Expand All @@ -679,7 +736,7 @@ class URLSearchParams {
value = String(value);
this[searchParams].push(name, value);
update(this[context], this);
}
},

delete(name) {
if (!this || !(this instanceof URLSearchParams)) {
Expand All @@ -700,47 +757,7 @@ class URLSearchParams {
}
}
update(this[context], this);
}

set(name, value) {
if (!this || !(this instanceof URLSearchParams)) {
throw new TypeError('Value of `this` is not a URLSearchParams');
}
if (arguments.length < 2) {
throw new TypeError('"name" and "value" arguments must be specified');
}

const list = this[searchParams];
name = String(name);
value = String(value);

// If there are any name-value pairs whose name is `name`, in `list`, set
// the value of the first such name-value pair to `value` and remove the
// others.
var found = false;
for (var i = 0; i < list.length;) {
const cur = list[i];
if (cur === name) {
if (!found) {
list[i + 1] = value;
found = true;
i += 2;
} else {
list.splice(i, 2);
}
} else {
i += 2;
}
}

// Otherwise, append a new name-value pair whose name is `name` and value
// is `value`, to `list`.
if (!found) {
list.push(name, value);
}

update(this[context], this);
}
},

get(name) {
if (!this || !(this instanceof URLSearchParams)) {
Expand All @@ -758,7 +775,7 @@ class URLSearchParams {
}
}
return null;
}
},

getAll(name) {
if (!this || !(this instanceof URLSearchParams)) {
Expand All @@ -777,7 +794,7 @@ class URLSearchParams {
}
}
return values;
}
},

has(name) {
if (!this || !(this instanceof URLSearchParams)) {
Expand All @@ -795,7 +812,47 @@ class URLSearchParams {
}
}
return false;
}
},

set(name, value) {
if (!this || !(this instanceof URLSearchParams)) {
throw new TypeError('Value of `this` is not a URLSearchParams');
}
if (arguments.length < 2) {
throw new TypeError('"name" and "value" arguments must be specified');
}

const list = this[searchParams];
name = String(name);
value = String(value);

// If there are any name-value pairs whose name is `name`, in `list`, set
// the value of the first such name-value pair to `value` and remove the
// others.
var found = false;
for (var i = 0; i < list.length;) {
const cur = list[i];
if (cur === name) {
if (!found) {
list[i + 1] = value;
found = true;
i += 2;
} else {
list.splice(i, 2);
}
} else {
i += 2;
}
}

// Otherwise, append a new name-value pair whose name is `name` and value
// is `value`, to `list`.
if (!found) {
list.push(name, value);
}

update(this[context], this);
},

// https://heycam.github.io/webidl/#es-iterators
// Define entries here rather than [Symbol.iterator] as the function name
Expand All @@ -806,7 +863,7 @@ class URLSearchParams {
}

return createSearchParamsIterator(this, 'key+value');
}
},

forEach(callback, thisArg = undefined) {
if (!this || !(this instanceof URLSearchParams)) {
Expand All @@ -827,7 +884,7 @@ class URLSearchParams {
list = this[searchParams];
i += 2;
}
}
},

// https://heycam.github.io/webidl/#es-iterable
keys() {
Expand All @@ -836,16 +893,17 @@ class URLSearchParams {
}

return createSearchParamsIterator(this, 'key');
}
},

values() {
if (!this || !(this instanceof URLSearchParams)) {
throw new TypeError('Value of `this` is not a URLSearchParams');
}

return createSearchParamsIterator(this, 'value');
}
},

// https://heycam.github.io/webidl/#es-stringifier
// https://url.spec.whatwg.org/#urlsearchparams-stringification-behavior
toString() {
if (!this || !(this instanceof URLSearchParams)) {
Expand All @@ -854,37 +912,14 @@ class URLSearchParams {

return querystring.stringify(getObjectFromParams(this[searchParams]));
}
}
// https://heycam.github.io/webidl/#es-iterable-entries
URLSearchParams.prototype[Symbol.iterator] = URLSearchParams.prototype.entries;

URLSearchParams.prototype[util.inspect.custom] =
function inspect(recurseTimes, ctx) {
const separator = ', ';
const innerOpts = Object.assign({}, ctx);
if (recurseTimes !== null) {
innerOpts.depth = recurseTimes - 1;
}
const innerInspect = (v) => util.inspect(v, innerOpts);

const list = this[searchParams];
const output = [];
for (var i = 0; i < list.length; i += 2)
output.push(`${innerInspect(list[i])} => ${innerInspect(list[i + 1])}`);
});

const colorRe = /\u001b\[\d\d?m/g;
const length = output.reduce(
(prev, cur) => prev + cur.replace(colorRe, '').length + separator.length,
-separator.length
);
if (length > ctx.breakLength) {
return `${this.constructor.name} {\n ${output.join(',\n ')} }`;
} else if (output.length) {
return `${this.constructor.name} { ${output.join(separator)} }`;
} else {
return `${this.constructor.name} {}`;
}
};
// https://heycam.github.io/webidl/#es-iterable-entries
Object.defineProperty(URLSearchParams.prototype, Symbol.iterator, {
writable: true,
configurable: true,
value: URLSearchParams.prototype.entries
});

// https://heycam.github.io/webidl/#dfn-default-iterator-object
function createSearchParamsIterator(target, kind) {
Expand All @@ -898,7 +933,9 @@ function createSearchParamsIterator(target, kind) {
}

// https://heycam.github.io/webidl/#dfn-iterator-prototype-object
const URLSearchParamsIteratorPrototype = Object.setPrototypeOf({
const URLSearchParamsIteratorPrototype = Object.create(IteratorPrototype);

defineIDLClass(URLSearchParamsIteratorPrototype, 'URLSearchParamsIterator', {
next() {
if (!this ||
Object.getPrototypeOf(this) !== URLSearchParamsIteratorPrototype) {
Expand Down Expand Up @@ -937,7 +974,7 @@ const URLSearchParamsIteratorPrototype = Object.setPrototypeOf({
done: false
};
},
[util.inspect.custom]: function inspect(recurseTimes, ctx) {
[util.inspect.custom](recurseTimes, ctx) {
const innerOpts = Object.assign({}, ctx);
if (recurseTimes !== null) {
innerOpts.depth = recurseTimes - 1;
Expand Down Expand Up @@ -968,15 +1005,6 @@ const URLSearchParamsIteratorPrototype = Object.setPrototypeOf({
}
return `${this[Symbol.toStringTag]} {${outputStr} }`;
}
}, IteratorPrototype);

// Unlike interface and its prototype object, both default iterator object and
// iterator prototype object of an interface have the same class string.
Object.defineProperty(URLSearchParamsIteratorPrototype, Symbol.toStringTag, {
value: 'URLSearchParamsIterator',
writable: false,
enumerable: false,
configurable: true
});

function originFor(url, base) {
Expand Down
20 changes: 13 additions & 7 deletions test/parallel/test-whatwg-url-tostringtag.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,24 @@ const toString = Object.prototype.toString;

const url = new URL('http://example.org');
const sp = url.searchParams;
const spIterator = sp.entries();

const test = [
[toString.call(url), 'URL'],
[toString.call(sp), 'URLSearchParams'],
[toString.call(Object.getPrototypeOf(sp)), 'URLSearchParamsPrototype'],
[url, 'URL'],
[sp, 'URLSearchParams'],
[spIterator, 'URLSearchParamsIterator'],
// Web IDL spec says we have to return 'URLPrototype', but it is too
// expensive to implement; therefore, use Chrome's behavior for now, until
// spec is changed.
[toString.call(Object.getPrototypeOf(url)), 'URL']
[Object.getPrototypeOf(url), 'URL'],
[Object.getPrototypeOf(sp), 'URLSearchParams'],
[Object.getPrototypeOf(spIterator), 'URLSearchParamsIterator'],
];

test.forEach((row) => {
assert.strictEqual(row[0], `[object ${row[1]}]`,
`${row[0]} !== [object ${row[1]}]`);
test.forEach(([obj, expected]) => {
assert.strictEqual(obj[Symbol.toStringTag], expected,
`${obj[Symbol.toStringTag]} !== ${expected}`);
const str = toString.call(obj);
assert.strictEqual(str, `[object ${expected}]`,
`${str} !== [object ${expected}]`);
});