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

Jump to bottom

url: check forEach callback is a function #10905

Closed
wants to merge 6 commits into from
Closed

url: check forEach callback is a function #10905

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f0f65de
url: add additional tests for URLSearchParams#forEach
TimothyGu Jan 15, 2017
44acced
url: check forEach callback is a function
TimothyGu Jan 15, 2017
182ce97
More specific error message tests
TimothyGu Jan 20, 2017
c6f5900
Remove obsolete arguments.length check in forEach()
TimothyGu Jan 20, 2017
6775629
Make argument check error messages consistent with rest of code base
TimothyGu Jan 20, 2017
593a0d9
Update error messages in test
TimothyGu Jan 21, 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
3 changes: 3 additions & 0 deletions lib/internal/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -817,6 +817,9 @@ class URLSearchParams {
if (arguments.length < 1) {
throw new TypeError('The `callback` argument needs to be specified');
}
if (typeof callback !== 'function') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this make the check on line 817 obsolete?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cjihrig, this provides more granular error messages, which I think is a good thing. Do you prefer me to just use the same error message for both cases?

In the browser, they have different error messages as well:

>> new URLSearchParams().forEach()
** TypeError: Failed to execute 'forEach' on 'URLSearchParams':
   1 argument required, but only 0 present.
>> new URLSearchParams().forEach(2)
** TypeError: Failed to execute 'forEach' on 'URLSearchParams':
   The callback provided as parameter 1 is not a function.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would remove the extraneous check as suggested

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our error messages don't even seem to match up though. I'd still change it, but I won't fight it too much.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed.

throw new TypeError('The `callback` argument must be a function');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our standard way of writing this would be TypeError('"callback" argument must be a function')

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

}

let list = this[searchParams];

Expand Down
12 changes: 12 additions & 0 deletions test/parallel/test-whatwg-url-searchparams.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,18 @@ n = 0;
for (val of sp.values()) {
assert.strictEqual(val, String(values[n++]));
}
n = 0;
sp.forEach(function(val, key, obj) {
assert.strictEqual(this, undefined);
assert.strictEqual(key, 'a');
assert.strictEqual(val, String(values[n++]));
assert.strictEqual(obj, sp);
});
sp.forEach(function() {
assert.strictEqual(this, m);
}, m);
assert.throws(() => sp.forEach(), TypeError);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of the TypeError constructor, can you pass a regular expression like /^TypeError: The callback argument must be a function$/

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

assert.throws(() => sp.forEach(1), TypeError);

m.search = '?a=a&b=b';
assert.strictEqual(sp.toString(), 'a=a&b=b');