feat(event-emitter): Enable invoking asynchronous handlers

[DavidHavl]

This update contains the following

Added:

• New emitAsync method to the EventEmitter to enable invoking asynchronous handlers.
• Added prevention for potential memory leak when adding handlers inside middleware via on method.
• Introduced new option of EventEmitter maxHandlers that limits number of handlers that can be added to a single event.

Changed:

• Significantly improved documentation.

[changeset-bot]

🦋 Changeset detected

Latest commit: 67f0d9f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@hono/event-emitter	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[yusukebe]

Hi @DavidHavl

I know using --immutable --immutable-cache is better, but can we keep using the following now? Or is CI failing?

yarn install --frozen-lockfile

[DavidHavl]

Hi @yusukebe , ok thank you,
I now reverted it back to --frozen-lockfile.

[yusukebe]

Hi @DavidHavl !

It's my question, like going back to square one. Is it possible to not pass Context as an argument to c.get('emitter').emit()?

c.get('emitter').emit('user:created', user)

Because using both c.get... and passing c seems verbose to users. What do you think of it?

[DavidHavl]

Thank you for your question @yusukebe.
It is a good one.

Firstly, as far as I understand, a new context is created for every request.
Secondly, there is only one instance of emitter (there is no state inside of it that needs to change with each new request so that seems the best approach) and is reused for every request.
With these 2 facts, the emitter does not have access to current context unless we give it to it.

The only way I can think of doing what you are asking is to create new instance of emitter for each request, but that seemed pointless to me as too many objects/instances would be flying around at same time for not a good reason.
Also the issue with that is that the emitter can be used as standalone (be initiated via const emitter = createEmitter() instead of as middleware) so there is no easy way to pull the context when initializing.

Initially, I was proposing to send (or not) the context via payload, but I really liked your suggestion few weeks ago to explicitly send the context as extra argument to the emit method. it nicely coincides with the rest of the framework and other middleware.

Here is a code that shows usage of the emitter (using standalone option) in real application:
'https://github.com/DavidHavl/hono-rest-api-starter/blob/main/src/events.ts'
'https://github.com/DavidHavl/hono-rest-api-starter/blob/95a2f1aed441cebd23aae1c0d073798d59981b46/src/features/team/bootstrap.ts#L40'
'https://github.com/DavidHavl/hono-rest-api-starter/blob/main/src/features/team/events/listeners.ts'

Please let me know if you have other questions.

[yusukebe]

@DavidHavl Thank you for your explanation!

but I really liked your suggestion few weeks ago to explicitly send the context as extra argument to the emit method. it nicely coincides with the rest of the framework and other middleware.

Yeah. I also think passing a context is good.

I have an idea, though I don't know whether it's possible or not and whether it's a good design or not.

First, define Emiter to allow having Context in the context field:

export interface Emitter<EventHandlerPayloads> {
  context: Context | undefined
  on<Key extends keyof EventHandlerPayloads>(
    key: Key,
    handler: EventHandler<EventHandlerPayloads[Key]>
  ): void
  off<Key extends keyof EventHandlerPayloads>(
    key: Key,
    handler?: EventHandler<EventHandlerPayloads[Key]>
  ): void
  emit<Key extends keyof EventHandlerPayloads>(key: Key, payload: EventHandlerPayloads[Key]): void
  emit<Key extends keyof EventHandlerPayloads>(
    key: Key,
    c: Context,
    payload: EventHandlerPayloads[Key]
  ): void
}

The definition of emit will be like the following:

emit<Key extends keyof EventHandlerPayloads>(
  key: Key,
  payloadOrContext: EventHandlerPayloads[Key] | Context,
  payload?: EventHandlerPayloads[Key]
) {
  // ...
  const handlerArray = handlers.get(key as EventKey)
  if (handlerArray) {
    for (const handler of handlerArray) {
      handler(context, realPayload)
    }
  }
}

And set a context in the middleware. Or we can create a setter method instead of accessing the property.

export const emitter = <EventHandlerPayloads>(
  eventHandlers?: EventHandlers<EventHandlerPayloads>
): MiddlewareHandler => {
  // Create new instance to share with any middleware and handlers
  const instance = createEmitter<EventHandlerPayloads>(eventHandlers)
  return createMiddleware(async (c, next) => {
    instance.context = c
    c.set('emitter', instance)
    await next()
  })
}

What do you think of this? If you feel it's nonsense, you can ignore it!

[DavidHavl]

@yusukebe I apologize for late reply.
Thank you for your idea.
I understand what you tried to achieve and especially like the overloaded function idea, but
the last part instance.context = c; c.set('emitter', instance);, would not work because new Context is created for each request, so if there are several concurrent requests, one would overwrite the previous one (and some handlers would have wrong data).

The possible way to achieve the adding Context when initializing emitter is to have the emitter be "request scoped", meaning a new instance of Emitter would be created for each request and destroyed when the request ends.
I have been thinking about this approach for a long time but I don't have a good feeling about it, because even though it solves some issues it introduces potentially more serious ones.

Here are some pros and cons of "request scoped" event emitter that I can think of:

Pros:

• Ability to add anonymous and closure functions as handlers INSIDE of middleware or request handlers.
• No need to pass context to each 'emit' or 'emitAsync' method
• It provides cleaner separation of concerns and better isolation between requests

Cons:

• Standalone version would have to have different documentation (requiring context in emit method arguments)
• Strain on Javascript's garbage collector (cleaning after each instance is not used anymore)
• Potentially consuming dangerously more memory especially when using closures or referencing large data structures within handlers.

I did also consider options such as "request-scoped event registration only", instance pooling", or "lazy initialization", but none of these would solve the issues sufficiently.

Please le me know if you have any questions or suggestions.

[yusukebe]

@DavidHavl

I understand what you tried to achieve and especially like the overloaded function idea, but
the last part instance.context = c; c.set('emitter', instance);, would not work because new Context is created for each request, so if there are several concurrent requests, one would overwrite the previous one (and some handlers would have wrong data).

Yeah, you are right. So, let's go with it.

One thing. You've changed the API for emit() that makes ac as a first argument:

c.get('emitter').emit(c, 'user:created', user)

Is this okay? This middleware was released a few months ago, and there are not so many users, but it will be breaking change. It's up to you, but if you want to follow a semver and don't want to bump a major, I think it's better not to add this change.

[DavidHavl]

@yusukebe Thank you for the response.
Yes, I have changed the context to be first argument. It was an oversight on my part before. It felt ok when I released it first, but after using Hono and the ecosystem much more now, it always hits me in the eye that the context is not a first argument like it is in other middleware or helpers like 'setCookie(c, 'cookiename', data);'.
And since I have added a big new feature (async handlers support) it feels kind of justifiable to bump to a major version.

[DavidHavl]

@yusukebe Any thoughts?

[yusukebe]

LGTM!

[yusukebe]

Hi @DavidHavl

Sorry for the late reply! Looks good. I'll merge this and release a new version now. Thank you!