-
Notifications
You must be signed in to change notification settings - Fork 30k
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.
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
events: implement EventEmitter#getMaxListeners() #82
Conversation
Returns the current max listener value for the emitter which is either set by | ||
`emitter.setMaxListeners(n)` or defaults to `EventEmitter.defaultMaxListeners`. | ||
|
||
This can be usefull to increment/decrement max listeners to avoid the warning |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/usefull/useful
Maybe clarify what warning? I know the warning but might be better to specify.
def554f
to
dfe3ecf
Compare
@kenany Thanks for feedback. I've addressed your issues. About clarifying the warning. Yes, I think that should be done, but I think it is out of scope for this pull request. |
@@ -67,6 +67,14 @@ EventEmitter.prototype.setMaxListeners = function setMaxListeners(n) { | |||
return this; | |||
}; | |||
|
|||
EventEmitter.prototype.getMaxListeners = function getMaxListeners() { | |||
if (!util.isUndefined(this._maxListeners)) { | |||
return this._maxListeners; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about returning Infinity
when this._maxListeners
is 0 to make more semantic, actually setMaxListeners(Infinity)
is working well, too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was thinking about that. It will return Infinity
if you have done setMaxListeners(Infinity)
. But it is documented that 0
means unlimited and I think being concise with existing api is best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
haha, yup, but the Infinity
in fact is better, because both setMaxListeners(Infinity)
and setMaxListeners(0)
are working always. perhaps any thoughts from TC?
Some style issues but otherwise LGTM. I'd like one more person to sign off on this. |
dfe3ecf
to
9de06aa
Compare
I've fixed all comments so far. The only thing missing is if it should return |
I would just make it return zero. Intuitively, I would expect I'll land this PR assuming tests pass. If someone feels strongly about the return value, they can follow up with a PR of their own. |
LGTM. |
Landed in 0115a5e. Thanks, Christian! |
Fixes nodejs/node-v0.x-archive#8237. PR-URL: #82 Reviewed-By: Ben Noordhuis <[email protected]>
Sorry about that, I screwed up the commit. 2931348 it is. |
…recompiled_vfsVfsNet While working with NodeJS tests it was found that some of them (tests) requires access to /etc/hosts file and so the getaddrinfo() call falls to error. The quote from developers: > The issue was nailed down to the ramdisk access for the entity which > implements network access (precompiled_vfsVfsNet). As appropriated > partition hasn't been mounted for it (so it is not exists for it). The > access to the ramdisk is covered by another entity, called > precompiled_vfsVfsRamFs. So it is required to configure (allow) access > for the precompiled_vfsVfsNet to the ramdisk over explicit properties. > > Besides of that it is required to add the 'hosts' file to the romfs > image, to make it accessible by application over /etc mount from romfs. TFS: nodejs#82 Signed-off-by: Vadim Lomovtsev <[email protected]>
fixes nodejs/node-v0.x-archive#8237