Make emit() wait for all listeners to settle

[sindresorhus]

See the initial attempt and feedback in #19.

[vitaly-t]

I think it is a bad idea, for emit to support it. I did some studying for this back when I was working on sub-events, and I believe that the only useful accumulative function that emit should do is to provide a way to handle errors thrown or rejected by the receivers.

And so I implemented it here, where for the list of all recipients I execute a regular forEach, and inside in I invoke the recipient's callback function inside try,catch, while also checking the result in case it is a rejected promise.

try {
        const res = sub.cb?.(data);
        if (typeof res?.catch === 'function') {
           res.catch((err: any) => onError(err, sub.name));
        }
} catch (e) {
      onError(e, sub.name);
}

This way my emit method can report into option onError both synchronous and asynchronous errors reported by the event recipients, which lets you implement safe event emission.

Other than that, the idea of making emit actually wait for the result - is never going to work. Because for one thing, the event recipient isn't bound to return a promise when it is doing asynchronous event handling, and least of all chain event processing into returned promise. It's just not practically feasible, because event handlers should be allowed a no-signature approach.

[mmkal]

Are the requirements for this finalised? There was some discussion on that PR about whether .emit() should be able to reject. I think there are arguments for both sides, but it could be a big breaking change to swallow errors. I can imagine a scenario where some 'startup' event throws because a precondition failed. It could be bad news if an application still tried to stand up after that - even if there should be better handling, in real life it might not work out that way. Off the top of my head, maybe a (recommended) constructor parameter could solve without breaking anyone?

const goodHandler = () => Promise.resolve(123)
const badHandler = () => Promise.reject(456)

const e1 = new Emittery() // no error event name declared - `.emit()` will reject when a handler throws as before
const e2 = new Emittery({ error: 'error' }) // explicitly declare what event should be used for errors. Solves the 'error' string vs Symbol debate too

e1.on('foo', goodHandler)
e2.on('foo', goodHandler)

e1.on('foo', badHandler)
e2.on('foo', badHandler)

await e1.emit() // throws WrappedError([{ status: 'fulfilled', value: 123 }, { status: 'rejected', reason: 456 }])
await e2.emit() // doesn't throw, but emits 'error' event WrappedError as above

Implementation-wise, this seems a good candidate for Promise.allSettled (or a polyfill since this package supports node 10) - which is where the Array<{ status: 'fulfilled, value: T } | { status: 'rejected', value: E }> format above comes from.

[sindresorhus]

From the linked PR:

That is to say, I'm not convinced listeners should be able to impact whether await this.emit('event') throws. It makes it harder to write predictable APIs if they can suddenly throw third-party errors.

I think if we allocate a new Promise.rejected(err) value that we don't return, then we can let Node.js' "unhandled rejection" infrastructure handle these cases. Alternatively Emittery could have a separate event that should be subscribed to so that errors can be logged etc, but application developers probably won't know to utilize that anyway.

...

I think what sindresorhus was arguing is that listener errors should be re-emitted as errors, as long as there's listeners for that event. If not they should be unhandled rejections.

I don't think listeners should (indirectly) influence what events are emitted. That implies separate infrastructure for application developers to be able to capture these errors. But that's tricky too since they may not be aware of Emittery or there may be multiple copies of emittery loaded into memory. [...] IMHO the best approach is to leave these rejections unhandled, without influencing the promise returned by emit().

This is what I think we should do.

[KATT]

Hey, I just found this old thread while thinking of adopting emittery.

Would it make sense for emit() to never throw and instead return Promise<null | [unknown, ...unknown]>?

import Emittery from 'emittery';

const emitter = new Emittery<{
  '🦄': string;
}>();

emitter.on('🦄', () => {
  // works
});

emitter.on('🦄', () => {
  throw new Error('💣');
});

const result = await emitter.emit('🦄', 'a');

if (result) {
  // result would be [Error('💣')]
  console.error('Errors', result);
} else {
  console.log('all OK!');
}