Skip to content
This repository has been archived by the owner on Apr 22, 2023. It is now read-only.

EventEmitter: move constructor guts to EventEmitter.init #6693

Closed
wants to merge 1 commit into from

Conversation

piscisaureus
Copy link

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

@Nodejs-Jenkins
Copy link

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):

  • First line of commit message must be no longer than 50 characters

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.
@piscisaureus
Copy link
Author

^-- Okay, jankins, there you go.

@trevnorris
Copy link

I now realize that both this and our current implementation is broken. If a user runs EventEmitter(); then it's going to add all the object properties to global. That's incorrect.

}
}
this._events = this._events || {};
this._maxListeners = this._maxListeners || undefined;

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.

Copy link
Author

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.

Copy link
Author

Choose a reason for hiding this comment

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

Also see cf0fa96

Copy link
Author

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.

@trevnorris
Copy link

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 .domain object property all the time.

@piscisaureus
Copy link
Author

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

@trevnorris
Copy link

LGTM

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants