events: support EventTarget in once

[benjamingr]

Add support for EventTarget in the once static method, hopefully this makes it a bit more universal.

Ping @mcollina please take a look.

(I can also revert the if the error change is to big)

[mcollina]

Would you mind adding some docs?

[apapirovski]

I think this currently doesn't quite do the right thing? Left some notes.

[apapirovski]

I could be going totally crazy but I feel like this function is incorrect and the test only passes because it runs through the EventEmitter path? I think you need something more like this?

function once(emitter, name) {
  return new Promise((resolve, reject) => {
    if (typeof emitter.once === 'function' &&
        typeof emitter.removeListener === 'function') {
      const eventListener = (...args) => {
        if (errorListener !== undefined) {
          emitter.removeListener('error', errorListener);
        }
        resolve(args);
      };
      let errorListener;

      // Adding an error listener is not optional because
      // if an error is thrown on an event emitter we cannot
      // guarantee that the actual event we are waiting will
      // be fired. The result could be a silent way to create
      // memory or file descriptor leaks, which is something
      // we should avoid.
      if (name !== 'error') {
        errorListener = (err) => {
          emitter.removeListener(name, eventListener);
          reject(err);
        };

        emitter.once('error', errorListener);
      }

      emitter.once(name, eventListener);
      return;
    }

    if (typeof emitter.addEventListener === 'function') {
      // EventTarget does not have `error` event semantics like Node
      // EventEmitters, we do not listen to `error` events here.
      emitter.addEventListener(name, resolve, { once: true });
      return;
    }

    throw new ERR_INVALID_ARG_TYPE('emitter', 'EventEmitter', emitter);
  });
}

[benjamingr]

I am not sure what the difference here - but this can be a brainfart on my behalf

[apapirovski]

Your original PR still binds the error listener (and unnecessarily creates the eventListener function). It also creates a nested Promise that it then resolves so the original never resolves. I could also be missing something... but when I ran the code, it didn't seem to do what you're expecting.

[benjamingr]

@apapirovski ah wow you're right!

[apapirovski]

Add emitter.removeListener = undefined; to hit the right code path in this test?

[apapirovski]

Worth mentioning the different argument semantics? It resolves with the Event instance rather than an array of arguments.

[benjamingr]

@apapirovski I was not sure about what to do here - what behavior would you think is less surprising?

[apapirovski]

More saying we should make clear the distinction in arguments that the callback receives when you use this with EventTarget.

[apapirovski]

Extra Promise not needed here?

[TimothyGu]

Also check that there are no interesting error semantics?

[benjamingr]

Superceded by #29498 (review) :]