-
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
Introduce EventEmitter#listenerCount and deprecate EventEmitter.listenerCount #734
Comments
I'm not aware of the history that led into this, but I'd like to see an addition like this and deprecation of |
The history is that because people inherit from |
@trevnorris can you link to the original discussion? I couldn't find it. |
@Fishrock123 First conversation: http://logs.nodejs.org/libuv/2013-03-01#22:49:38.745 (notice that at that time it was called |
Ok, I don't see any discussion on why it shouldn't / couldn't be on the prototype though. (Which was what I was hoping for haha) |
Basically people add all sorts of things to the EventEmitter prototype, and adding something could break user-land code. |
Has anyone done even a cursory check to see what level of conflicts would arise with listenerCount on the prototype? It's crazy to think we could never ever iterate and improve an API once it's released just because people inherit from it. |
@jasonkarns no, that's in progress but nobody has done something like that on EE afaik (@Fishrock123?). I agree that it's crazy - but only because we allow additions for other inheritable objects, with EE being the only exception. I've raised an issue about this, asking for clarification: nodejs/dev-policy#76 |
@trevnorris Atm the Events API is not stated as being «Locked», btw. |
@ChALkeR And underscored methods are "private", but doesn't prevent us from reverting changes b/c the community abuses it. ;-P |
As per the discussion in nodejs#734, this patch deprecates the usage of `EventEmitter.listenerCount` static function in the docs, and introduces the `listenerCount` function in the prototype of `EventEmitter` itself. PR-URL: nodejs#2349
As per the discussion in #734, this patch deprecates the usage of `EventEmitter.listenerCount` static function in the docs, and introduces the `listenerCount` function in the prototype of `EventEmitter` itself. PR-URL: #2349 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
As per the discussion in #734, this patch deprecates the usage of `EventEmitter.listenerCount` static function in the docs, and introduces the `listenerCount` function in the prototype of `EventEmitter` itself. PR-URL: #2349 Reviewed-By: Trevor Norris <trev.norris@gmail.com> Reviewed-By: Brian White <mscdex@mscdex.net>
Done in 8f58fb9 |
The
listenerCount
method was introduced (75305f3) for performance reasons afterEventEmitter#listeners
started to return a copy of the listeners array (nodejs/node-v0.x-archive#3442). It was added as a static method because of problems with backwards compatibility when thedomain
field was introduced toEventEmitter
(nodejs/node-v0.x-archive#5310).Now that we are somewhat less strict about this and is introducing other new methods (2931348) on the prototype for
EventEmitter
, maybe we could clean up this api?(You guys who was involved in this, feel free to correct me on the historical details)
The text was updated successfully, but these errors were encountered: