events: add/removeListener should call on/off

[ronag]

Previously these used alias which caused surprising behaviour
when trying to override add/removeListener, e.g. Readable.

This resolves this by making on/off actually call the
corresponding function through the prototype chain.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[addaleax]

I would leave the documentation as it is, because .on() is still the most common way to attach an event listener to an emitter.

[ronag]

@addaleax: Leaving it as is will be wrong though? If you override on it won't actually do what it says. You should always be overriding addListener.

[addaleax]

Then maybe we could add notes to the on/off docs that say “This function is implemented by calling addListener()/removeListener()”?

[ronag]

Then maybe we could add notes to the on/off docs that say “This function is implemented by calling addListener()/removeListener()”?

Sure.

But currently the documentation says that addListener is an Alias for emitter.on(eventName, listener). Which is strictly untrue. So I'm not sure how to apply your suggestion.

I'm not sure how to keep it close to as it was, and at the same time make it correct.

[benjamingr]

this is a bit scary compared to making it an alias just for streams. I am fine with this with a citgm run :]

[ronag]

This is potentially dangerous. Depending on which one is overriden and how one could end up in infinite recursion.

I'm very unsure whether this is worth landing... alternatively we could update the docs to make sure both are override and maybe add some kind of warning, i.e. if one is override the other must also be overriden?

[ronag]

@addaleax I sorted out your suggestion. By making on and removeListener primary (i.e. how they are used in the wild).

[ronag]

Can we solve this better using getters/setters or Proxy?

[ronag]

@Trott: would you mind adding a WIP on this so it doesn't land.

[Fishrock123]

Does this duplicate the work in #29486?

[ronag]

@Fishrock123: Yes, but this has risks while #29486 does not. I think this PR needs significant more thought.

As I wrote above, if we're unlucky we can cause infinite recursion depending on which methods are and are not overriden.

[devsnek]

but then you'd notice the range error from the stack overflow and fix it, right?

[ronag]

but then you'd notice the range error from the stack overflow and fix it, right?

Yes, but if we are unlucky it could break stuff in the ecosystem pretty significantly. Is this worth it?�

[devsnek]

if you're worried about a specific situation currently in the ecosystem, we can run citgm and gzemnid code searches to see if this will break/interfere.

[ronag]

I'm fine with that. Just making sure we are aware of it :). This should be a server-major?

@devsnek: Are you still approving given the latest changes? If so please remove the WIP label.

[ronag]

I think it we should have off call removeListener instead. This would greatly reduce the risk of breakage in old modules.

I can see the point. However, the problem there is that it becomes very confusing when you have to override on/removeListener instead of on/off or addListener/removeListener. I can see a lot of future bugs and confusion from that.

Any ideas on how to get the best of both?

[benjamingr]

An alternative might be to provide a util on events that does something like:

Events.overrideEmitterMethods(myEmitterLike, {
  on(...) { /* override on*/ },
  off(...) { /* override off*/ }
});

And tell people they can use that. We can also add a note to the docs about the historical reasons leading to the events API and the ecosystem.

[mcollina]

That'd work and it might not be a semver-major change. Essentially if you don't use it, you are bound to do the multiple override yourself.

[benjamingr]

So just making sure - we agree on:

• Ronag's work here with Matteo's suggestion above
• People override on/removeListener but don't actually have to do that because we provide a utility method.

Please let me know if I am understanding this correctly.
Cheers and thanks 🙇

[ronag]

@benjamingr: Including my work here will be sem-major (in my opinion) even with the utility function.

We could add just the utility as sem-minor.

[benjamingr]

@ronag that SGTM

[ronag]

Utility as separate PR?

[ronag]

Fixed to follow @mcollina suggestion, i.e. on/removeListener which seems to be the least breaking version of this.

[devsnek]

having a utility is the same problem as having to manually do the alias like the readable pr does. if you just do class X extends EventEmitter { on() {} } you're broken.

Maybe we can check new.target.prototype.on and new.target.prototype.addEventListener and etc and see if they've been overridden?

[ronag]

We could do it like streams does, i.e:

class X extends EventEmitter {
  constructor () {
    super({
      on() {},
      off() {}
    })
  }
}

i.e. send the overrides as constructor arguments.

[ronag]

EDIT: Not a good idea

A crazy idea that might be non breaking and fixes the issue. Choose the implementation from the deepest prototype chain in the constructor and override with a property.

Something like:

function EventEmitter() {
  // This should only be false the first time an instance
  // of this class is created.
  if (this.on !== this.addListener) {
    const proto = Object.getPrototypeOf(this)
    const [primary, secondary] = distance(proto, 'on') < distance(proto, 'addListener')
      ? ['on', 'addListener']
      : ['addListener', 'on']
    Object.defineProperty(proto, secondary, {
      get () {
        return this[primary]
      },
      set (value) {
        this[primary] = value
      }
    })
  }
}

function distance(proto, name) {
  let n = 0
  while (proto && !proto[name]) {
    n++
    proto = Object.getPrototypeOf(proto)
  }
  return n
}

Not sure of the performance implications though...

EDIT: Updated

[devsnek]

@ronag i think you could check for this.on === new.target.prototype.on and such

[ronag]

@devsnek: I'm not sure if that is enough, what if the override is one step up the chain?

[ronag]

Here is an alternative (that works) which dynamically patches the prototype: nxtedition#5.

It shouldn't be breaking and no docs update required. However, it's quite a hack and doesn't work if any properties are overriden on the instance or the prototype after construction.

[benjamingr]

doesn't work if any properties are overriden on the instance or the prototype after construction.

Unfortunately I believe that is quite common :/

@devsnek

having a utility is the same problem as having to manually do the alias like the readable pr does. if you just do class X extends EventEmitter { on() {} } you're broken.

That's true but note that the issue a utility would solve is not about incorrectly extending eventemitter and not overriding stuff like off - it is to provide a sanctioned way to override EventEmitter that doesn't encourage extending an assymetrical API (on/removeListener)

[mcollina]

Here is an alternative (that works) which dynamically patches the prototype: nxtedition#5.

I would prefer we had no code to be executed during creation of the EventEmitter, as a lot of implementations in the wild do not call the constructor.

[ronag]

So I guess the current state of this PR is the most favorable so far?

We should probably have overrideEmitterMethods as a separate PR?

[mcollina]

I'm not really convinced this is the way to go either. I would prefer to document the status quo first.

[jasnell]

Marking semver-major due to the possibility of breaking things.

[jasnell]

Just throwing this in for discussion...

Another potential way to do this would be to introduce the actual implementation behind a symbol function that both on/addListener (and likewise off/removeListener) would defer to. Then, the override could be just for that, but obviously that wouldn't work for any existing overriding out in the ecosystem.

This is just a quick example of what I mean...

const kOn = Symbol('kOn');
const kOff = Symbol('kOff');

EventEmitter.prototype[kOn] = function(event, handler) { / *** / };
EventEmitter.prototype[kOff] = function(event, handler) { / *** / };
EventEmitter.onSymbol = kOn;
EventEmitter.offSymbol = kOff;

EventEmitter.prototype.on = function(event, handler) { return this[kOn](event, handler); };
EventEmitter.prototype.addListener = function(event, handler) { return this[kOn](event, handler); };
EventEmitter.prototype.off = function(event, handler) { return this[kOn](event, handler); };
EventEmitter.prototype.removeListener = function(event, handler) { return this[kOn](event, handler); };


// Then users could override both using
EventEmitter.prototype[EventEmitter.onSymbol] = function...
EventEmitter.prototype[EventEmitter.offSymbol] = function...

Again, just throwing that option in for consideration. Not sure about the overall change.

[benjamingr]

Do we have any previous research on how event emitter overriding is typically done in userland?

I would assume it's just "people override whatever works and whatever they are using and it's chaotic" but that's just an assumption I am making and I would like to see if we can prove or disprove that.

[ronag]

@Trott I don't think I can make progress on this one. Needs more guidance. Should I leave this open and hope someone picks up on it or close?

[Trott]

@Trott I don't think I can make progress on this one. Needs more guidance. Should I leave this open and hope someone picks up on it or close?

I don't know that I have a good answer for that. Maybe one of @addaleax @benjamingr @mcollina @devsnek @jasnell @Fishrock123 would have clarity on the best approach.

[ronag]

@Trott: Should I close this for now? Maybe make an issue for it instead?