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

events, doc: add type checking in defaultMaxListeners getter #11938

Closed
wants to merge 1 commit into from
Closed

events, doc: add type checking in defaultMaxListeners getter #11938

wants to merge 1 commit into from

Conversation

DavidCai1111
Copy link
Member

@DavidCai1111 DavidCai1111 commented Mar 20, 2017 •
edited
Loading

Since EventEmitter.defaultMaxListeners and emitter.setMaxListeners(n) are both public APIs listed in the document, so it is reasonable to check EventEmitter.defaultMaxListeners getter's input like what emitter.setMaxListeners(n) does to avoid potential confusing results or error messages caused by some userland mistaken input, like:

'use strict'
const E = require('events')

E.defaultMaxListeners = new Proxy({}, { get: () => {} })
let e = new E()
e.addListener('1', function () {})
e.addListener('1', function () {})
// output:
// events.js:260
//      if (m && m > 0 && existing.length > m) {
//                 ^
//
// TypeError: Cannot convert object to primitive value
//     at _addListener (events.js:260:18)
//     at EventEmitter.addListener (events.js:279:10)
//     at Object.<anonymous> (/Users/caiwei/workspace/benchmarks/index.js:7:3)
//     at Module._compile (module.js:571:32)
//     at Object.Module._extensions..js (module.js:580:10)
//     at Module.load (module.js:488:32)
//     at tryModuleLoad (module.js:447:12)
//     at Function.Module._load (module.js:439:3)
//     at Module.runMain (module.js:605:10)
//     at run (bootstrap_node.js:425:7)
Checklist
  • [-] make -j4 test (UNIX), or vcbuild test (Windows) passes
  • [-] tests and/or benchmarks are included
  • [-] documentation is changed or added
  • [-] commit message follows commit guidelines
Affected core subsystem(s)

events, doc

@nodejs-github-bot nodejs-github-bot added the events Issues and PRs related to the events subsystem / EventEmitter. label Mar 20, 2017
mscdex
mscdex reviewed Mar 20, 2017
View reviewed changes
lib/events.js Outdated
// force global console to be compiled.
// see https://github.com/nodejs/node/issues/4467
console;
defaultMaxListeners = arg;
if (typeof n !== 'number' || n < 0 || isNaN(n))
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we replace isNaN(n) with n !== n, possibly with a comment with a short explanation?

Copy link
Contributor

@vsemozhetbyt vsemozhetbyt Mar 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@mscdex Is this similar place also worth replacing then?

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably, but as a separate commit I'd say.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mscdex @vsemozhetbyt OK, updated.

@DavidCai1111 DavidCai1111 force-pushed the events/add-default-max-listeners-type-checking branch from aa568e6 to 58f7610 Compare March 20, 2017 14:33
@DavidCai1111
Copy link
Member Author

@vsemozhetbyt Thanks for reminding, changed it to a shorter one :)

@vsemozhetbyt
Copy link
Contributor

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

@DavidCai1111 DavidCai1111 force-pushed the events/add-default-max-listeners-type-checking branch 2 times, most recently from ce92c12 to 35db7e6 Compare March 20, 2017 15:35
@vsemozhetbyt
Copy link
Contributor

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

mscdex
mscdex reviewed Mar 20, 2017
View reviewed changes
lib/events.js Outdated
// force global console to be compiled.
// see https://github.com/nodejs/node/issues/4467
console;
defaultMaxListeners = arg;
// check whether the input is a positive number (which value is
// larger than zero and not a NaN).
Copy link
Contributor

Choose a reason for hiding this comment

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

'which value is larger than zero' should probably instead be something like 'whose value is zero or greater'

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Mar 20, 2017
cjihrig
cjihrig approved these changes Mar 20, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a couple nits.

doc/api/events.md Outdated
@@ -262,7 +262,8 @@ By default, a maximum of `10` listeners can be registered for any single
event. This limit can be changed for individual `EventEmitter` instances
using the [`emitter.setMaxListeners(n)`][] method. To change the default
for *all* `EventEmitter` instances, the `EventEmitter.defaultMaxListeners`
property can be used.
property can be used. If trying to set it to a value which is not a positive
Copy link
Contributor

Choose a reason for hiding this comment

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

Can I suggest rewording this as:

If this value is not a positive number, a TypeError will be thrown.

lib/events.js Outdated
@@ -52,11 +52,15 @@ Object.defineProperty(EventEmitter, 'defaultMaxListeners', {
get: function() {
return defaultMaxListeners;
},
set: function(arg) {
set: function(n) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Changing this variable name seems unnecessary.

jasnell
jasnell approved these changes Mar 20, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM with nits addressed

@DavidCai1111 DavidCai1111 force-pushed the events/add-default-max-listeners-type-checking branch from 35db7e6 to b3c06f2 Compare March 21, 2017 00:20
@DavidCai1111
Copy link
Member Author

@mscdex @cjihrig Thanks for reviewing. Just updated, PTAL. 😃

@mscdex
Copy link
Contributor

mscdex commented Mar 21, 2017

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

@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

@mscdex ... does this LGTY?

mscdex
mscdex reviewed Mar 22, 2017
View reviewed changes
lib/events.js
// check whether the input is a positive number (whose value is zero or
// greater and not a NaN).
if (typeof arg !== 'number' || arg < 0 || arg !== arg)
throw new TypeError('defaultMaxListeners must be a positive number');
Copy link
Contributor

@mscdex mscdex Mar 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

'defaultMaxListeners' should be enclosed in double quotes to match the style when referencing property names in error messages.

@mscdex
Copy link
Contributor

mscdex commented Mar 22, 2017

One minor nit, but otherwise LGTM.

@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

I can take care of that nit while landing

jasnell pushed a commit that referenced this pull request Mar 22, 2017
@DavidCai1111 @jasnell
events, doc: check input in defaultMaxListeners
221b03a 
PR-URL: #11938
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Brian White <[email protected]>
@jasnell
Copy link
Member

jasnell commented Mar 22, 2017

Landed in 221b03a

@jasnell jasnell closed this Mar 22, 2017
@jasnell jasnell mentioned this pull request Apr 4, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@vsemozhetbyt vsemozhetbyt vsemozhetbyt left review comments

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@hiroppy hiroppy hiroppy approved these changes

Assignees
No one assigned
Labels
events Issues and PRs related to the events subsystem / EventEmitter. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@DavidCai1111 @vsemozhetbyt @mscdex @jasnell @lpinca @cjihrig @hiroppy @nodejs-github-bot