Skip to content
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

Closed
tellnes opened this issue Feb 5, 2015 · 11 comments
Closed
Labels
events Issues and PRs related to the events subsystem / EventEmitter.

Comments

@tellnes
Copy link
Contributor

tellnes commented Feb 5, 2015

The listenerCount method was introduced (75305f3) for performance reasons after EventEmitter#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 the domain field was introduced to EventEmitter (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)

@brendanashworth brendanashworth added the events Issues and PRs related to the events subsystem / EventEmitter. label Feb 6, 2015
@brendanashworth
Copy link
Contributor

I'm not aware of the history that led into this, but I'd like to see an addition like this and deprecation of EventEmitter.listenerCount(..). A static-style function such as this doesn't belong in an API that is built around prototype'd methods. 👍 from me.

@trevnorris
Copy link
Contributor

The history is that because people inherit from EventEmitter it was decided that we wouldn't screw with any additional prototype properties. My original PR for listenerCount() was on the prototype, but that was shut down.

@Fishrock123
Copy link
Contributor

@trevnorris can you link to the original discussion? I couldn't find it.

@trevnorris
Copy link
Contributor

@Fishrock123 First conversation: http://logs.nodejs.org/libuv/2013-03-01#22:49:38.745 (notice that at that time it was called listenerLength) and a follow up: http://logs.nodejs.org/libuv/2013-08-12#12:12:50.802

@Fishrock123
Copy link
Contributor

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)

@trevnorris
Copy link
Contributor

@Fishrock123

22:50:32    <isaacs>     bnoordhuis: otoh, API is "locked", and people raise holy hell when we add fields to EE
22:50:39    <bnoordhuis> exactly
22:50:48    <bnoordhuis> i don't think new fields are really an option at this time
22:50:59    <isaacs>     bnoordhuis: are you saying, not even underscored?
22:51:14    <isaacs>     hm. we can put it on the root events object.
22:51:17    <bnoordhuis> well no, because people use underscored properties themselves
22:51:17    <isaacs>     or EventEmitter class
22:51:27    <isaacs>     EventEmitter.listenerLength(emitter)
22:51:37    <bnoordhuis> yes, namespaced off somewhere safe

Basically people add all sorts of things to the EventEmitter prototype, and adding something could break user-land code.

@jasonkarns
Copy link
Member

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.

@brendanashworth
Copy link
Contributor

@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

@ChALkeR
Copy link
Member

ChALkeR commented Aug 16, 2015

@trevnorris Atm the Events API is not stated as being «Locked», btw.

@trevnorris
Copy link
Contributor

@ChALkeR And underscored methods are "private", but doesn't prevent us from reverting changes b/c the community abuses it. ;-P

thefourtheye added a commit to thefourtheye/io.js that referenced this issue Aug 19, 2015
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
thefourtheye added a commit that referenced this issue Aug 19, 2015
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>
thefourtheye added a commit that referenced this issue Aug 20, 2015
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>
@Fishrock123
Copy link
Contributor

Done in 8f58fb9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
events Issues and PRs related to the events subsystem / EventEmitter.
Projects
None yet
Development

No branches or pull requests

6 participants