-
Notifications
You must be signed in to change notification settings - Fork 7.3k
EventEmitter: move constructor guts to EventEmitter.init #6693
Conversation
Thank you for contributing this pull request! Here are a few pointers to make sure your submission will be considered for inclusion. Commit 2f8d21d has the following error(s):
You can fix all these things without opening another issue. Please see CONTRIBUTING.md for more information |
After landing 6ed861d it is no longer possible to reliably monkey-patch the EventEmitter constructor. However there's valid use cases for that, and makes for easier debugging. Therefore, move the guts of the constructor to a separate function which is monkey-patchable.
^-- Okay, jankins, there you go. |
I now realize that both this and our current implementation is broken. If a user runs |
} | ||
} | ||
this._events = this._events || {}; | ||
this._maxListeners = this._maxListeners || undefined; |
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.
This isn't necessary. this._maxListeners
is already set to undefined
by the prototype.
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.
I copied this code literally from the original constructor. I think it's better for performance to have the property on the object itself.
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.
Also see cf0fa96
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.
If you care, however, I'll drop that line in one go :). Just say the word.
IMO, we shouldn't consider "monkey-patching" a useful or good practice. This is still a little ways off, but hence why I'm finishing up #6502. It's going to be necessary anyways to fix the edge cases from using AsyncListeners, and it provides a performance gain not having to check the |
@trevnorris Not a good or useful practice per se. But it's very useful for debugging or for making new stuff work in older versions. Think about the asyncListener polyfill that Tim has done, for example. |
LGTM |
After landing 6ed861d it is no longer possible to reliably monkey-patch
the EventEmitter constructor. However there's valid use cases for that,
and makes for easier debugging. Therefore, move the guts of the
constructor to a separate function which is monkey-patchable.
The idea was signed off by @isaacs already. Here's the implementation.
suggested reviewers: @trevnorris @tjfontaine