FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

url: check forEach callback is a function by TimothyGu · Pull Request #10905 · nodejs/node · GitHub

/ node Public

url: check forEach callback is a function - #10905

Closed
TimothyGu wants to merge 6 commits into
nodejs:masterfrom
TimothyGu:urlsearchparams-foreach
ConversationCommits (6)ChecksFiles changed
Closed

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

Conversation

TimothyGu 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 defines forEach as the following:

void forEach(Function callback, optional any thisArg);

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

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

cjihrig Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Does this make the check on line 817 obsolete?

TimothyGu Jan 20, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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.

jasnell Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I would remove the extraneous check as suggested

cjihrig Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

TimothyGu Jan 20, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Changed.

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

cjihrig Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

TimothyGu Jan 20, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

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

TimothyGu commented Jan 20, 2017

Copy link
Copy Markdown
Member Author

/cc @nodejs/url

TimothyGu force-pushed the urlsearchparams-foreach branch from 981f558 to 182ce97 Compare January 20, 2017 02:04
targos reviewed Jan 20, 2017
View reviewed changes
Comment thread lib/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');

targos Jan 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

TimothyGu Jan 20, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Fixed.

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

TimothyGu commented Jan 22, 2017

Copy link
Copy Markdown
Member Author

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

TimothyGu commented Jan 24, 2017

Copy link
Copy Markdown
Member Author

Landed in ed0086f.

TimothyGu closed this Jan 24, 2017
TimothyGu deleted the urlsearchparams-foreach branch January 24, 2017 00:44
TimothyGu added a commit that referenced this pull request Jan 24, 2017
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
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>
italoacasas mentioned this pull request Jan 25, 2017
Closed
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 27, 2017
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>
italoacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
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
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell left review comments
cjihrig cjihrig approved these changes
joyeecheung joyeecheung approved these changes
+1 more reviewer
targos targos approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

whatwg-url Issues 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

Back | FazBrowse Home | New Git URL