Skip to content

url: check forEach callback is a function - #10905

Closed
TimothyGu wants to merge 6 commits into
nodejs:masterfrom
TimothyGu:urlsearchparams-foreach
Closed

url: check forEach callback is a function#10905
TimothyGu wants to merge 6 commits into
nodejs:masterfrom
TimothyGu:urlsearchparams-foreach

Conversation

@TimothyGu

@TimothyGuTimothyGu commented Jan 20, 2017

Copy link
Copy Markdown
Member

Currently, URLSearchParams' forEach function does not have a check to make sure that the type of the provided callback is a function. However, the Web IDL spec, to which this function should conform, insists that a TypeError be thrown if the callback is not callable.

This PR fulfills that requirement. Also included in this PR are additional tests for certain aspects of forEach that are defined in the spec but not yet tested.

Web IDL definesforEach as the following:

voidforEach(Functioncallback, optionalanythisArg);

callback therefore has type Function, which in turn is defined as a callback function:

callbackFunction=any(any...arguments);

The process to convert an ECMAScript value to an IDL callback function then contains the following:

  1. If the result of calling IsCallable(V) is false and the conversion to an IDL value is not being performed due to V being assigned to an attribute whose type is a nullable callback function that is annotated with [TreatNonObjectAsNull], then throw a TypeError.

And since callback is not declared with [TreatNonObjectAsNull], forEach should throw a TypeError in case callback is not a function.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

url

@nodejs-github-botnodejs-github-bot added dont-land-on-v4.x whatwg-url Issues and PRs related to the WHATWG URL implementation. labels Jan 20, 2017
cjihrig
cjihrig reviewed Jan 20, 2017
View reviewed changes
Comment threadlib/internal/url.js
if (arguments.length < 1) {
throw new TypeError('The `callback` argument needs to be specified');
}
if (typeof callback !== 'function') {

@cjihrigcjihrigJan 20, 2017

Copy link
Copy Markdown
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?

@TimothyGuTimothyGuJan 20, 2017

Copy link
Copy Markdown
MemberAuthor

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:

>>newURLSearchParams().forEach()**TypeError: Failedtoexecute'forEach'on'URLSearchParams':
1argumentrequired,butonly0present.>>newURLSearchParams().forEach(2)**TypeError: Failedtoexecute'forEach'on'URLSearchParams':
Thecallbackprovidedasparameter1isnotafunction.

@jasnelljasnellJan 20, 2017

Copy link
Copy Markdown
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

@cjihrigcjihrigJan 20, 2017

Copy link
Copy Markdown
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.

@TimothyGuTimothyGuJan 20, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Changed.

cjihrig
cjihrig reviewed Jan 20, 2017
View reviewed changes
Comment threadtest/parallel/test-whatwg-url-searchparams.js Outdated
sp.forEach(function() {
assert.strictEqual(this, m);
}, m);
assert.throws(() => sp.forEach(), TypeError);

@cjihrigcjihrigJan 20, 2017

Copy link
Copy Markdown
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$/

@TimothyGuTimothyGuJan 20, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fixed.

@TimothyGuTimothyGu mentioned this pull request Jan 20, 2017
Closed
11 tasks
@TimothyGu

TimothyGu commented Jan 20, 2017

Copy link
Copy Markdown
MemberAuthor

/cc @nodejs/url

@TimothyGu
TimothyGuforce-pushed the urlsearchparams-foreach branch from 981f558 to 182ce97CompareJanuary 20, 2017 02:04
targos
targos reviewed Jan 20, 2017
View reviewed changes
Comment threadlib/internal/url.js Outdated
throw new TypeError('The `callback` argument needs to be specified');
}
if (typeof callback !== 'function') {
throw new TypeError('The `callback` argument must be a function');

@targostargosJan 20, 2017

Copy link
Copy Markdown
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')

@TimothyGuTimothyGuJan 20, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Fixed.

@TimothyGuTimothyGu mentioned this pull request Jan 22, 2017
Closed
3 tasks
@TimothyGu

TimothyGu commented Jan 22, 2017

Copy link
Copy Markdown
MemberAuthor

CI: https://ci.nodejs.org/job/node-test-pull-request/5985/

@TimothyGu

TimothyGu commented Jan 24, 2017

Copy link
Copy Markdown
MemberAuthor

Landed in ed0086f.

@TimothyGu
TimothyGu deleted the urlsearchparams-foreach branch January 24, 2017 00:44
TimothyGu added a commit that referenced this pull request Jan 24, 2017
@TimothyGu
url: check forEach callback is a function
ed0086f
The Web IDL spec mandates such a check.
Also make error messages consistent with rest of Node.js and add
additional tests for forEach().
PR-URL: #10905
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 25, 2017
@TimothyGuItalo A. Casas
url: check forEach callback is a function
b20912a
The Web IDL spec mandates such a check.
Also make error messages consistent with rest of Node.js and add
additional tests for forEach().
PR-URL: nodejs#10905
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@italoacasasitaloacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
@TimothyGuItalo A. Casas
url: check forEach callback is a function
df85d7c
The Web IDL spec mandates such a check.
Also make error messages consistent with rest of Node.js and add
additional tests for forEach().
PR-URL: nodejs#10905
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@italoacasasitaloacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@TimothyGuItalo A. Casas
url: check forEach callback is a function
95faa55
The Web IDL spec mandates such a check.
Also make error messages consistent with rest of Node.js and add
additional tests for forEach().
PR-URL: nodejs#10905
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
@TimothyGuItalo A. Casas
url: check forEach callback is a function
3052c2e
The Web IDL spec mandates such a check.
Also make error messages consistent with rest of Node.js and add
additional tests for forEach().
PR-URL: nodejs#10905
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell left review comments
@targostargostargos approved these changes
@cjihrigcjihrigcjihrig approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes

Assignees

No one assigned

Labels

whatwg-urlIssues and PRs related to the WHATWG URL implementation.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@TimothyGu@jasnell@targos@cjihrig@joyeecheung@mscdex@nodejs-github-bot