EventEmitter API

[ChALkeR]

Moved from #1785 (comment)

Some modules use the internal _events object. It's not a good thing, and that probably means that EventEmitter is missing some API methods.

Also lib/_stream_readable.js uses the internal _events object of lib/events.js, which is ok, but not very nice. What makes that a bit worse is that lib/_stream_readable.js is also packaged as an external readable-stream module.

Samples of _events usage:

• readable-stream/lib/_stream_readable.js:
  1. if (!dest._events || !dest._events.error)
  2. else if (isArray(dest._events.error))
  3. dest._events.error.unshift(onerror);
  4. dest._events.error = [onerror, dest._events.error];
• dicer/lib/Dicer.js:
  1. if (this._events.preamble)
  2. if ((start + i) < end && this._events.trailer)
  3. if (this._events[ev])
• busboy/lib/types/multipart.js:
  1. if (!boy._events.file) {
• ultron/index.js:
  1. for (event in this.ee._events) { if (this.ee._events.hasOwnProperty(event)) {

It looks to me that there should be methods in EventEmitter to:

• Get a list of all events from an EventEmmiter that currently have listeners. This is what ultron module does, and I do not see an API for that. Getting a list of events could also be usable for debugging.

  Implemented by @jasnell in events: add eventNames() method #5617, will be available in 6.0.

• (optional) Check if there are any listeners for a specific event. Like EventEmitter.listenerCount but using a prototype, like EventEmitter.prototype.listeners but returning just the count. This is what seems to be most commonly used.

  It has a valid API EventEmitter.listenerCount, but why isn't it inside the prototype? That makes it a bit harder to find and that could be the reason behind modules using this._events.whatever to count (or check the presense of) the event listeners.

  That's since 75305f3. @trevnorris

  Solved by events: deprecate static listenerCount function #2349.

• (optional) To prepend an event. Does the documentation specify the order in which the events are executed, btw?

  This is what the internal lib/_stream_readable.js and the readable-stream module do.

  Implemented by @jasnell in events: add ability to prepend event listeners #6032.

[ChALkeR]

From #1785 (comment) discussion:
@mscdex @chrisdickinson @sam-github @Qard

[targos]

Existing discussion about EventEmitter#listenerCount: #734

[ChALkeR]

@chrisdickinson

readable-stream is directly vendored from core's streams. Core has a need for prepending events – userland does not (with the exception of readable-stream, which doesn't seem like a good excuse to add it.)

Actually, it does. readable-stream is a widely used (actually, that's a bad thing) module that uses internal iojs API. That, for example, blocks the internal EventEmmiter API changes, see #1785 (comment).

Either the usage of _events by the readable-stream module should be fixed, or the whole situaton where everyone uses readable-stream module (that is pretty much abadonware atm) should be somehow fixed.

[chrisdickinson]

@ChALkeR It doesn't block changing the API, it blocks changing _events. We can always shim it back in using a getter/setter property.

[ChALkeR]

@chrisdickinson Ok, I might have misphrased that. I meant the internal details of the EventEmmiter module. Yes, a shim would fix that. And it will be needed in the current situation once #1785 (or something else that changed _events) get pulled. But doesn't a shim for an internal object look a bit …strange? The root of problem here is that modules have to use the internal _events object to do stuff.

If you don't want to expose an API to do what readable-stream does (see 3.) — fine, that could be understandable (well, because that's technically a hack now).

But I see nothing wrong in adding an API endpoint that allows getting a list of all events that have listeners (see 1.).

[chrisdickinson]

But doesn't a shim for an internal object look a bit …strange?

It's a cowpath, for better or worse. It'd be interesting to get some metrics on how pervasive its use is in the ecosystem, but given readable-stream's dependency on it I'm inclined to say it's probably worth preserving (vs. patching out of the ecosystem.) Updating all libraries dependent on readable-stream of any version would be a pretty intense exercise.

With regards to "getting a list of event topic names with listeners" from an EventEmitter – outside of Ultron, are there any other modules that make use of _events for this purpose? If so, it's probably OK to add an EventEmitter.getEventNames(ee) API.

[trevnorris]

@ChALkeR Reason it wasn't placed on the prototype is because EE is inherited by everything, and it was decided at the time that adding a new method could break compatibility. (IIRC there was a module pointed out that uses that same method name)

[Qard]

A possible way around that is a non-prototype method like
events.getEventNames(emitter).
On May 27, 2015 6:20 PM, "Trevor Norris" notifications@github.com wrote:

@ChALkeR https://github.com/ChAlKeR Reason it wasn't placed on the
prototype is because EE is inherited by everything, and it was decided at
the time that adding a new method could break compatibility. (IIRC there
was a module pointed out that uses that same method name)

—
Reply to this email directly or view it on GitHub
#1817 (comment).

[rektide]

I showed up in nodejs/io.js to grieve about not having a real way to do @ChALkeR's #1 use case. I had written up streaming-heart-mother to kind-of/sort-of work around this: it listens to newListener events and exposes an enumeration of them via a knownEvents property. (It also monkey-patches .emit to look for events happening with no subscribers)

This has been a very longstanding issue I've had with the DOM's EventTarget interface, and which EventEmitter repeated- the un-introspectability, the inability to see what's happening on the target site without getting there first and monkey-patching the hay out of it to keep a record for yourself. A good, reflective first-class system would put this right and expose the key information about the object, not retain it from the programmers: looking at _events is a completely incorrect hack that is just a faster, better, clearner version of (my, others) filthy monkey patching.

(There's GetEventListeners, which is something available in Developer Tools, which seems 1/3rd as pleasant still as emitter._events)

The EventEmitter.prototype API ought be amended so differing implementations of events can successfully cooperate together. _events was never specified, and isn't the right interface- EventEmitter.prototype.getEventNames feels to me much closer to the desired interface. Doing events.getEventNames would only make things 100x worse than the current state by obliterating a internal but well-known contract of the object and replacing it with a static method specific to the implementation: this would be the opposite of what is prescribed in a slot-based/predicated language.

It's high time EventTarget's original high treason against good JS object-ness be corrected.

[rektide]

There is still one use case I'd like to be able to solve- both dynamic writers and dynamic listeners. For example: if there's a emitter that knows how to read from a variety of kafka queues, and listeners that might want to pick certain queues (listen to events) if the name matches some predicate, then both sides are waiting for the other to either emit or listen, and neither knows whether to tentatively begin bindings/offering itself first. It's a wait-locked situation. It's "solvable" by having the emitter either once or periodically emit zero-sized content events, by being proactive, and letting monkey-patch logic let the listener-side see these emits happening, but just as _events is a coordination point that developers ought to have reach into, so to is emit a source-of-information that enriches the environment.

So in contrast to my Streaming Heart Mother v1.1, which can convey knownEvents on the emitter, a fantastic, out of this world awesome api would be able to let both producers find all listeners, and listeners explore all possible emits systems. It's a dual that I'd like to see filled, of allowing both sides of the emitter to express what capabilities are about (possible emitters, as well as listed listeners). But I get that perhaps this might better be reserved for a higher level collaboration api.

[ChALkeR]

Ah, and @3rd-Eden for ultron.

[ChALkeR]

About (3), there are modules for that:

1. https://www.npmjs.com/package/on-first. Uses _events internally, basically the same code as in readable-stream. But this module isn't popular and there are no packages that depend on that, it seems.
2. https://www.npmjs.com/package/overshadow-listeners Also not popular and 0 dependants, but what is intresting here — it applies a different kind of hack that does not involve _events and uses only the existing public EventEmmiter API. Works by reattaching and attaching existing listeners.

Maybe the readable-stream could be ported away from using _events directly to use the same method as overshadow-listeners uses? That could remove the need in (3) completely.

[chrisdickinson]

@rektide If we added such a reflection API, we'd likely put it on the constructor itself (a la EE.listenerCount, for the same reasons.) While I feel your pain about the impurity of having what should be a per-instance method available only as a static method that operates on instances, in this case avoiding breaking downstream code trumps aesthetic concerns.

@ChALkeR Even if we fix readable-stream such that _events are no longer used directly, we still have to release those changes in a patch release. Then we have to run through each of the 612 dependents of readable-stream and find the ones that have pinned the dep, and issue a PR to the new version (& make a release.) Then for each of the dependents of those dependents, we have to issue a PR for every pinned dep there. It's probably going to be more expedient to shim the _events API, treating it as a discouraged, but paved, cowpath.

[ChALkeR]

@chrisdickinson Shimming _events is required in the current situaton if #1785 or something else breaking _events gets merged.

But to remove the bloat in the long-term, usage of _events in modules (including readable-stream) should be reduced as soon as possible.

Maybe if we do that now and if EventEmmiter internal structure would be changed in, for example, 4.0 — it could be fine to avoid shimming _events.

Edit: There is another PR that breaks the internal _events api: #914.

[3rd-Eden]

I will always prefer internal "private" objects over API methods for code that runs in hot path. But given the fact that there are a lot of modules (public and private) that are already using _events I would even consider marking it as public and frozen. Just my 2 cents.

[ChALkeR]

@3rd-Eden, _events would be slower than a proper API when _events will be turned into a getter/setter-based shim.

[rektide]

Trying again. Whatever member getEventNames or _events (which would be fine, if specified)- whether it's on the prototype or publicly on the Object, is fine. So long as there's something formal specified accessible from the Object that people can use to lookup events, it's all good. (And looking at the api docs I do now notice that most methods are held by the emitter, not the EventEmitter.protoype; that's fine- my mistake asking for anything on prototypes previously).

I do think a getEventNames would be ideal to standardize around; it plus the existing listeners(event) is sufficient to read out all data from _events, be it a Object or Map, no? That's what make what seems like a good pair to me; it is the most minimal API addition which will yield up the data we see existing use cases relying on. Map#get : Map#keys :: Emitter#listeners : Emitter#getEventNames

[3rd-Eden]

Then don't turn it in a getter/setter...

Arnout Kazemier

On May 28, 2015, at 9:22 AM, Сковорода Никита Андреевич notifications@github.com wrote:

@3rd-Eden, _events would be slower than a proper API if _events will be turned into a getter/setter-based shim.

—
Reply to this email directly or view it on GitHub.

[ChALkeR]

@3rd-Eden Alternatives?

[bnoordhuis]

What's wrong with turning _events into an accessor that complains noisily about using an internal property? That will motivate module authors to update their code.

The real events object/array can then be hidden behind an ES6 symbol and, after some time has passed, be changed into a Map or whatever if that is beneficial. Everyone wins, right?

[ChALkeR]

What's wrong with turning _events into an accessor that complains noisily about using an internal property? That will motivate module authors to update their code.

Sounds perfect to me.

@3rd-Eden proposed just the opposite thing in #1817 (comment), and I see no advantages in that approach ☺.

[rektide]

This is a real capability that people should have @bnoordhuis! @ChALkeR has taken the time to carefully show a variety of real use cases. Kindly make something usable for people with legitimate use cases that won't spew errors! I dunno- make a canonical events and have _events spew console messages if you want. Or add getEventNames and make _events spew. But please don't just put some seed over the cowpath, pitch up some new barb wire around it, and call it done. Us cows want to find a way to go this direction, and you should help rather than obstruct.

Introducing more jank because people HAD to use a janky internal feature and calling this done: please please no.

I understand the desire to let things settle some before starting to make public apis. A getEventNames() iterable seems like a minimal addition that any future implementation can easily and without burden provide. I'd love to see it added. Crossed with listeners(event), it allows for an abstract implementation (perhaps an implementation throwing warnings) of _event, which seems ideal.

[bnoordhuis]

Principle of proportionality: if a handful of people are inconvenienced by something that benefits everyone else, it's usually a worthwhile tradeoff. (Although if you take that brand of utilitarianism to its logical extreme, you arrive at fifty years of torture.)

The tradeoff here is between the people that use _events directly versus everyone whose application get a nice speed boost when EE becomes more efficient. The question is of course where the crossover point is.

Okay, small digression aside, the request is for something that returns all events and all listeners? Or just event names? My initial suggestion would be to change EventEmitter.prototype.listeners to return a Map or Map-like object when called with no arguments.

[trevnorris]

@3rd-Eden Are there use cases in existing modules, that you know of, where operations are performed directly on _events?

[ChALkeR]

The current problem is that an internal _events property of the EventEmmiter is misused.
That is because:

1. Current API does not allow introspection of event listeners.
2. Some people are not aware of EventEmitter.listenerCount, possibly because it's not inside the prototype.
3. Some people tend to ignore the fact that using «private» properties is bad and could break anytime.
4. The readable-stream stuff and others (are there any?) that need their event handlers to be excuted before all the other ones.

(2,3) could be solved by using a getter-setter based shim for _events (temporary) and emmiting warnings from it, as @bnoordhuis proposed, so everyone will have enough time to port from using _events directly.

(1) could be solved by a new API method. It doesn't really matter if that would be inside the protoype (like EventEmitter.prototype.listeners) or outside (like EventEmitter.listenerCount), given that the warning would be there if someone instead uses _events directly.

(4) Probably doesn't need any new API methods because overshadow-listeners or similar techniques look fine. In fact, I propose using the same method in readable-stream. And the use-case «prepend an event listener» is technically a hack, could cause conflicts, and shouldn't be encouraged by introducing a public API method just for that.

[rektide]

@ChALkeR On (1), a new API method- EventEmitter.listenerCount makes interop between different event implementations impossible, and is very bad. If someone want to use EventEmitter3 and node events? Too bad-> footgun. This is just not proper JavaScript. Implementing an EventEmitter.whatever would be worse than this many-year old state of affairs where people are using an internal API that at least has been trivially specified and repeatable.

Migrating to an API which can not be recreated outside of Node would be brutal- a big ole wrapping up of the thing of barb wire to those who have regard for isomorphism, and respect javascript as a fine predicate-based loose-coupled language.

[chrisdickinson]

Re: the {g,s}etter approach – I'd advise against making it emit warnings for now. readable-stream has 0.5MM downloads a day, ~600 direct dependents, and is usually a third- or fourth- order dependency, making it difficult to fix the root problem for most users. We run the risk of flooding users with deprecation notices, making the deprecation notice system less useful as a result. The {g,s}etter approach does, however, let us cut our losses on the current internal implementation without breaking any downstream code. If we want to get rid of _events, we can put in the legwork to go patch usage in the ecosystem later.

Re: a new API for getting a list of event topics – I'm all for adding that API to the constructor. Similar APIs have been shimmed by browserify, so there's no worry about loosing isomorphism. Adding the API to the constructor is less likely to break existing code. Avoiding unnecessary (or aesthetic) downstream breakage is our primary concern.

[rektide]

@chrisdickinson it seems like the worst possible thing imaginable for introducing an api: add a singleton method specific to an implementation where interoperability will be impossible. Citing Browserify is a dodge: Browserify doesn't have an interop challenge, a it only has to make standalone (like Node, self-complete) replacement of Node's api. As I asked yesterday, please tell me how EventEmitter3 and Node's events would interop successfully around a method on the constructor? How would EventEmitter7 interop?

The dilemna we're stems from the old, old crime of the DOM API: not allowing EventTarget to be reflected on. Fix our defacto new better more widely adopted EventTarget object, and do an ok job, please. Not by introducing hacks that Node can happen to offer (but no one else will be able to interop with) entirely outside of the emitter object.

I don't understand the case for adding a method on the constructor, not the object: I've seen one mention of a performance concern, which I honestly don't begin to grasp, and other than that I see what seems like very smart people agreeing to put it on the constructor. And that terrifies the bejeesus out of me, because me makes me think the devs don't see or have respect as I do for Node as the project defining the Javascript platform (as well as being a great runtime). Fracturing vital APIs between objects and their singleton constructor is way way bad news, as far as platforming goes.

Again I ask: how is EventEmitter3.getPropertyNames (or whatever) supposed to interop with EventEmitter.getPropertyNames? If we have reason to use Constructor.method(self) (do we? I don't understand why we would do this), why do we have methods on objects at all in Node?

[chrisdickinson]

I don't understand the case for adding a method on the constructor, not the object: I've seen one mention of a performance concern, which I honestly don't begin to grasp, and other than that I see what seems like very smart people agreeing to put it on the constructor. And that terrifies the bejeesus out of me, because me makes me think the devs don't see or have respect as I do for Node as the project defining the Javascript platform (as well as being a great runtime). Fracturing vital APIs between objects and their singleton constructor is way way bad news, as far as platforming goes.

It's not a performance concern. The previous discussion around listenerCount is here, as @targos noted – specifically, event emitters are so widely used that adding new fields stands a high risk of breaking downstream user code – code that might be enumerating properties of an event emitter, or inheriting from an EE and expecting to have the name available, etc.

As it stands, _events won't be going away anytime soon – we are discussing turning it into a {g,s}etter property that exposes exactly what it exposes now so we have freedom to switch the internal format. A new API on the constructor does not change that. As for interoperation with EventEmitter3, EventEmitter3 can add a getEventNames method to its constructor (for parity) and continue looking at _events, for now. If we decide to deprecate and remove _events, we can revisit exposing the necessary data for that API to continue working at that point – we could likely add a Symbol-accessible prototype method at that point. Until we get to that point, doomsaying is counterproductive – _events will still be accessible, and it'll still be able to enumerate the appropriate event names for back-compatibility concerns. Also, keep in mind that a getEventNames API is net-new, and we haven't established that there is a wide use for it yet, outside of keeping people from using _events.

[benjamingr]

@3rd-Eden this post is about adding methods to event emitters. I was referring to that - not to emitting symbols.

[kanongil]

Some relevant history:

The readable-stream use of _events was introduced due to an unsolvable case reported in nodejs/node-v0.x-archive#4978 where I proposed an API extension to node to handle this cleanly. This was never merged, and eventually @isaacs decided to implement the hack in nodejs/node-v0.x-archive#6075.

This gist of the unsolvable, is that it is impossible to fake listen to the error event (preserving the no-listener throwing behavior) in a transparent way.

[simonkcleung]

One way to hide the internal properties is using WeakMap ( ES6), in this way , for example:

"use strict";
var EE=function(){
var events=new WeakMap();
class EE {
    constructor() {
        events.set(this, {});
    }
    on(event,listener){
        events.get(this)[event]=listener;
        return this;
    }
    emit(event){
        events.get(this)[event]();
    }
}
return EE;
}();

var e=new EE();
e.on("event",function(){
    console.log("Hello Event");
});

e.emit("event"); // "Hello Event"