catching errors requires await even when calling synchronous functions

[parthnaik]

const EventEmitter = require('promise-events')
const eventEmitter = new EventEmitter()

async function asyncFunction() {
    throw new Error('Async function error')
}

eventEmitter.on('callAsyncFunction', asyncFunction)

const runAsyncFunction = async() => {
    try {
        await eventEmitter.emit('callAsyncFunction')
    } catch(e) {
        console.log(e)
    }
}

runAsyncFunction() // Works as expected and prints 'Async function error'.


function syncFunction() {
    throw new Error('Sync function error')
}

eventEmitter.on('callSyncFunction', syncFunction) 

try {
    eventEmitter.emit('callSyncFunction') // Needs await if I want to catch the error even though it is calling a sync function to work as expected.
} catch(e) {
    console.log(e)
}
// Unhandled Promise Rejection. Requires await before eventEmitter.emit('callSyncFunction') even though calling sync function to allow it to catch the error.

Right now I am using the in-built node eventEmitter for synchronous functions and this one for async functions. If this is fixed, I can simply use this emitter and not use the in-built one at all.

The fix for this is actually simple since you are directly inheriting from the eventEmitter class. Instead of overriding its default emit method, let it be as is and rename your emit method to something else like emitAsync OR if you do not want to rename it, allow emit to accept an argument that allows it to run the default emit function.

[yanickrochon]

Hi @parthnaik ! Thank you for opening this issue. Unfortunately for you this is not something that can be fixed the way you suggest. The goal of this event emitter is to treat emit as asynchronous, so it is no longer synchronous and won't synchronously throw errors at all (unless some use case haven't been tested, in which case that would indeed require modifications).

So, you either have to use the emitter as in your first example, or use it as a Promise

function syncFunction() {
    throw new Error('Sync function error')
}

eventEmitter.on('callSyncFunction', syncFunction) 

eventEmitter.emit('callSyncFunction').catch(e => {
    console.log(e)
});

[parthnaik]

EDIT: Created a pull request with proposed change. Would appreciate a review and merge.

Hi @parthnaik ! Thank you for opening this issue. Unfortunately for you this is not something that can be fixed the way you suggest. The goal of this event emitter is to treat emit as asynchronous, so it is no longer synchronous and won't synchronously throw errors at all (unless some use case haven't been tested, in which case that would indeed require modifications).

So, you either have to use the emitter as in your first example, or use it as a Promise

function syncFunction() {
    throw new Error('Sync function error')
}

eventEmitter.on('callSyncFunction', syncFunction) 

eventEmitter.emit('callSyncFunction').catch(e => {
    console.log(e)
});

Hi @yanickrochon, thanks for the reply.

The fix is as simple as adding the line emitSync = events.EventEmitter.prototype.emit within your extended class. This doesn't change anything with regards to the emit method that you have implemented (or for that matter any of your other code) but yet allows the the original emit method to be accessible just with a different name.

If you are willing, I will create a pull request. It will not break anything in your code really and no additional tests will be required either since you are simply renaming the original method that was inherited from the events class.

class EventEmitter extends events.EventEmitter {
	emitSync = events.EventEmitter.prototype.emit // One line addition before the declaration of your custom emit. Will not break or change anything.
	// <rest of your code begins here>
}

It is your repo of course but don't see why you would decline this change request since it simply adds functionality without changing anything else.

[yanickrochon]

Thanks! I will review it, of course.

[parthnaik]

@yanickrochon I closed the pull request. Ended up solving my problem like follows:

const EventEmitter = require('events')
const AsyncEventEmitter = require('promise-events')

AsyncEventEmitter.prototype.emitAsync = AsyncEventEmitter.prototype.emit
AsyncEventEmitter.prototype.emit = EventEmitter.prototype.emit

const eventEmitter = new AsyncEventEmitter()

module.exports = eventEmitter

[yanickrochon]

In a way I'm glad that you found a solution because I had difficulties finding ways to justify the PR, on a engineering standpoint. I mean, a module A emits events, another module B listens to these events, module A should not manage the errors from module B! The goal of an event emitter is to remove dependencies between two modules, allow injection of responsibilities, but the emitter should not intercept errors from the listeners; module A did it's things, module A notifies that it's doing it's things, if module B has a problem with it, it should handle it in a graceful manner. If module B has a problem with module A, then module A should provide the API to resolve any issue, throwing errors should not be the solution for a control flow of processes.

In the end, you found a solution that works for you, and I'm happy if it works for you. But as far as this module is concerned, I personally do not see that solution as a good long term implementation, and would not recommend it. It's a matter of opinion, but I have many years backing this opinion.

Cheers.

[parthnaik]

In a way I'm glad that you found a solution because I had difficulties finding ways to justify the PR, on a engineering standpoint. I mean, a module A emits events, another module B listens to these events, module A should not manage the errors from module B! The goal of an event emitter is to remove dependencies between two modules, allow injection of responsibilities, but the emitter should not intercept errors from the listeners; module A did it's things, module A notifies that it's doing it's things, if module B has a problem with it, it should handle it in a graceful manner. If module B has a problem with module A, then module A should provide the API to resolve any issue, throwing errors should not be the solution for a control flow of processes.

In the end, you found a solution that works for you, and I'm happy if it works for you. But as far as this module is concerned, I personally do not see that solution as a good long term implementation, and would not recommend it. It's a matter of opinion, but I have many years backing this opinion.

Cheers.

Hey, just to continue this conversation. In my case module A is the server router and module B is a process that is run remotely on the server. The user has the ability to start/stop the process in module B hence I am emitting an event from module A (the router) which is listened for in module B (which then starts/stops the process).

Hence I am catching errors from module B in module A to convey to the user when something goes wrong (like when the process was unable to start). Since only module A (the server router) is responsible for communicating with the end user, I don't see a way by which I can eliminate communication between module A and module B.

I have used a similar pattern in reverse as well where if the user makes changes to the database from the router (module A), they need to be communicated immediately to module B (if the process is currently running), so module A will emit an event, which module B listens for and does the needful, in this case updating a local variable with the changes to the the database.

Is there a better way to design this? Would love to know. Thank you!

[yanickrochon]

Concerning your first use case, and it might also apply with your second, you can borrow from what the browser does with events, namely

Module A

let cancelled = false;
const event = {
  type: 'someEvent',
  get isCancelled() { return cancelled; },
  cancel() { cancelled = true; }
};

await events.emit('someEvent', event);

if (cancelled) {
  // some listener cancelled it!
  return;
}

// continue....

Module B

events.on('someEvent', async event => {
  // init...
  if (someCondition) {
    event.cancel();         // event.isCancelled will be true
  }
});

Since you're using this module, I assume that you are supporting async/await, otherwise why else would you want this EventEmitter?

In short, don't use errors to control your app. Errors should be for debugging, and for edge cases only. For example, React uses error to implement lazy loading of components. It does this because there is no component to display, so it errors and falls back to the default loading indicator until a refresh is triggered and attempted again. If no provider is used to wrap the component, then the error will bubble up and crash the app for debugging purposes. If React could implement this without throwing errors, they would; it's an edge case. But yours isn't; you can very well implement your use case without errors, just as we can stop the propagation of events in the DOM via the preventDefault() and stopPropagation() methods.

Hope this helps.

[parthnaik]

@yanickrochon Thanks, your comment led me down a rabbit hole and I did end up refactoring my code to make it more clean.