EventEmitter spec needs to be defined more clearly

[mattheworiordan]

See https://github.com/ably/ably-ios/pull/179/files#r51544655

[mattheworiordan]

We agreed that:

Generally I would prefer that when you call off(), or removeListener you specify criteria - listener and/or event - and all registered handlers matching the given criteria are removed. This makes the most consistent spec when looking at all combinations of event or listener being unspecified.

[paddybyers]

This makes the most consistent spec when looking at all combinations of event or listener being unspecified.

Removing only one seems to work but doesn't. In particular, suppose you do

emitter.on('event', f);
emitter.once('event', f);

Then you do

emitter.off('event', f);

which one gets removed?

[mattheworiordan]

Removing only one seems to work but doesn't. In particular, suppose you do

But we don't remove only one, in this PR the off method always removes all matching listeners?

[paddybyers]

But we don't remove only one, in this PR the off method always removes all matching listeners?

Yes. I'm saying that the alternative behaviour of removing only one, which node does, is harder to make useful and intuitive.

[mattheworiordan]

Ah, yes, OK, sorry I misunderstood. If you're happy with the PR for ably-js then I will update docs and close this issue

[tcard]

It's probably worth it to specify what happens when you register a listener more than once.

[paddybyers]

It's probably worth it to specify what happens when you register a listener more than once.

Ideally that would be a no-op - ie on the second call the listener doesn't get re-added, and when the event fires it calls the listener only once. To implement this adds to the cost of .on() but the alternatives are:

• add the listener a second time; IMO this is then not really consistent with requiring only a single off() to remove both;
• raise an error;
• undefined: we say that the API does not support that and you cannot make any assumptions about the result.

So I think the first option is better than the alternatives.

[tcard]

Ideally that would be a no-op - ie on the second call the listener doesn't get re-added, and when the event fires it calls the listener only once.

That's what I implemented at ably/ably-cocoa#179, basically for the first point in your list. (More concretely, the second on overrides the first if criteria conflicts.)

I was about to scratch that and mimic Node.js' behavior, should I leave it as it is?

[paddybyers]

(More concretely, the second on overrides the first if criteria conflicts.)

This is correct I think.

should I leave it as it is?

I think so, yes. Definitely don't "fix" it before we've finished this discussion.

[mattheworiordan]

I disagree. I think it's quite plausible that the same callback could be registered twice, and I don't see why we would ever want to dedupe that. I don't think any developer would ever expect that a listener registered twice would only ever fire once, that is just confusing and adds unnecessary complexity IMHO.

In the case of an off, I don't think there is any right way necessarily i.e. off could mean remove all listeners that match the listener, or could also mean remove one listener, however given there is no way to iterate listeners in our API and the ordering of which listener to remove is not that easy to guarantee, I think specifying that off always removes all listeners is just fine.

[tcard]

I don't think any developer would ever expect that a listener registered twice would only ever fire once

I would, if I thought that listeners are treated as elements in a set, which is what the off(listener) operation suggests. I know that it suggested that to me, for what it's worth. In list-like interfaces (with this I mean an interface that lets you add the same item more than once), I would expect an off operation to take some kind of index.

The fact that you got confused about off removing one or all instances of the listener suggests to me that it's a less natural choice of an API for that kind of behavior that it would be for a set-like (deduping) behavior.

[mattheworiordan]

I would, if I thought that listeners are treated as elements in a set, which is what the off(listener) operation suggests.

I don't really see it suggests that. Docs for eventListener for Node.js at least say removeListener will remove, at most, one instance of a listener from the listener array. So that's not a set but an array of registered listeners. Also, adding a listener method is on but has the alias addListener, not addOrUpdateListener etc. Docs say Adds the listener function to the end of the listeners array for the specified event. No checks are made to see if the listener has already been added. Multiple calls passing the same combination of event and listener will result in the listener being added, and called, multiple times.

I appreciate it could be interpreted in different ways, and I think the confusion lies with more off than on, but I still think calling on translates quite naturally to on this event call this callback, if someone wanted that to happen twice they would simply make that statement twice.

Saying all this, I question whether we should deviate from the Node.js implementation if we can't agree.

[paddybyers]

So that's not a set but an array of registered listeners.

Yes, but the point is that it is never addressable as an array; entries in the collection are only ever identifiable by their own identity (ie event and listener) which makes the abstraction more suggestive of a set.

[mattheworiordan]

Yes, but the point is that it is never addressable as an array;

Sorry, that's not true, https://nodejs.org/api/events.html#events_emitter_listeners_event returns an array of listeners.

Entries in the collection are only ever identifiable by their own identity (ie event and listener)

Well no, they have an index. In fact, they also are publicly exposed in the ably-js implementation, see https://github.com/ably/ably-js/blob/master/common/lib/util/eventemitter.js#L5-L8, which I personally dislike.

which makes the abstraction more suggestive of a set.

I could see that if the object was intended as a data store of some sort, but if it was, then it would expose suitable methods such as length, get, set etc. Our EventEmitter does not even allow discovery of registered event emitters because it's not trying to be that. It's providing an API that allows a developer to say "when this thing happens please call this method", or "I no longer want this method to be called when this thing happens". An API that lets you register more than one of the same listener seems right because if a developer says "when this thing happens please call this method", and later says it again somewhere else in their code, I think it's reasonable to assume they wanted that behaviour to happen again or else they wouldn't have asked us to call the method again. If we were going to treat it as a set, then we should simply reject that second request to avoid ambiguity or at least provide a new way of saying "will this method be called if this things happens", neither of which we can do now easily without changing the API. Saying "please don't call this method anymore when this event happens", IMHO, is fine to then remove all instances of that method that is registered for that event because they didn't have any way of specifying which one they wanted removed, and they asked us to not call the method so we're not.

Perhaps we should have a call to discuss tomorrow as we should finalise this either way.

[tcard]

We agreed on a call to go with the array-like behavior, but having off removing all matching instances of the same listener callback.

[mattheworiordan]

This can be closed now, see 1fb9c7f