events, doc: add type checking in defaultMaxListeners getter#11938
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added the events 
lib/events.js Outdated
There was a problem hiding this comment.
Could we replace isNaN(n) with n !== n, possibly with a comment with a short explanation?
There was a problem hiding this comment.
@mscdex Is this similar place also worth replacing then?
There was a problem hiding this comment.
Probably, but as a separate commit I'd say.
DavidCai1111force-pushed the events/add-default-max-listeners-type-checking branch from aa568e6 to 58f7610Compare
DavidCai1111 commented Mar 20, 2017
@vsemozhetbyt Thanks for reminding, changed it to a shorter one :) |
vsemozhetbyt commented Mar 20, 2017
DavidCai1111force-pushed the events/add-default-max-listeners-type-checking branch 2 times, most recently from ce92c12 to 35db7e6Comparevsemozhetbyt commented Mar 20, 2017
lib/events.js Outdated
There was a problem hiding this comment.
'which value is larger than zero' should probably instead be something like 'whose value is zero or greater'
jasnell added the semver-major 
doc/api/events.md Outdated
There was a problem hiding this comment.
Can I suggest rewording this as:
If this value is not a positive number, a TypeError will be thrown.
lib/events.js Outdated
There was a problem hiding this comment.
Changing this variable name seems unnecessary.
DavidCai1111force-pushed the events/add-default-max-listeners-type-checking branch from 35db7e6 to b3c06f2CompareDavidCai1111 commented Mar 21, 2017
mscdex commented Mar 21, 2017
jasnell commented Mar 22, 2017
@mscdex ... does this LGTY? |
| // check whether the input is a positive number (whose value is zero or | ||
| // greater and not a NaN). | ||
| if(typeofarg!=='number'||arg<0||arg!==arg) | ||
| thrownewTypeError('defaultMaxListeners must be a positive number'); |
There was a problem hiding this comment.
'defaultMaxListeners' should be enclosed in double quotes to match the style when referencing property names in error messages.
mscdex commented Mar 22, 2017
One minor nit, but otherwise LGTM. |
jasnell commented Mar 22, 2017
I can take care of that nit while landing |
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 commented Mar 22, 2017
Landed in 221b03a |
9 participants
Since
EventEmitter.defaultMaxListenersandemitter.setMaxListeners(n)are both public APIs listed in the document, so it is reasonable to checkEventEmitter.defaultMaxListenersgetter's input like whatemitter.setMaxListeners(n)does to avoid potential confusing results or error messages caused by some userland mistaken input, like:Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
events, doc