Have event handler callbacks support both async/sync methods

[JstnMcBrd]

Which application or package is this feature request for?

discord.js (and any others where applicable)

Feature

The recent discord.js update has caused me to start having errors from @typescript-eslint because the listener callbacks of event listener methods do not accept asynchronous methods. Here's a good example:

// Set the client application commands and then exit the client
client.once('ready', async (c) => {
  await c.application.commands.set(commands);
  await c.destroy();
});

Promise returned in function argument where a void return was expected. eslint(@typescript-eslint/no-misused-promises)

Documentation

That's because the type of the client.once method is:
Client.once<Event>(event: Event, listener: (client: Client) => void): Client

As you can see, the listener callback only accepts methods that return void, but not Promise<void>, effectively blocking any asynchronous methods.

Ideal solution or implementation

I would like to request that the types of all event listeners be changed to accept both async and non-async methods.

Here's what I would suggest:

  once<Event>(
    event: Event,
-   listener: (...args: any[]) => void
+   listener: (...args: any[]) => void | Promise<void>
  ): Client;

And the same for all other event handler methods.

This would be an easy change that would not affect behavior at all. Async methods do not need to be awaited by the event handler - it just needs to accept them and call them. This would maintain identical behavior between using async methods and using sync methods with promise chaining inside.

I'm submitting this as a feature request instead of a bug because it technically isn't a bug - discord.js is working as intended and is not obligated to accept async methods. But I would like it to, so I am requesting it as a feature.

Alternative solutions or implementations

One alternate solution would be to rewrite all of my callback methods using .then() promise chaining, but I think that would look ugly. I would rather be able to use an async method with await statements.

Another solution would be to disable that particular @typescript-eslint rule. But it's a good rule that has prevented me from writing bugs with async logic.

Other context

No response

[Qjuh]

They do support that. The issue is within your linter config here disallowing something that‘s perfectly valid typescript. See https://www.typescriptlang.org/docs/handbook/2/functions.html#return-type-void
See #10017 for details on why this was changed.

[didinele]

Hi! This rule serves in spotting plenty of difficult bugs to deal with. In this case, when a callback is typed with an expected return type of void, it implies that, if a Promise were to be returned, it wouldn't be awaited, which is absolutely correct. The event handling in discord.js is standard EventEmitter (in some subpackages Vlad's async emitters).

One side effect you may not be aware of, is that multiple callback calls can be in effect in parallel. Think of this (pseudo-code) web example:

let someState;
htmlElement.on('click', async () => {
  someState = await someAsyncFetchCall();
});

This is a disaster! If the user clicks multiple times on the button, the JS engine leaves each invocation waiting for the hypothetical HTTP call to end, and it's possible they finish in an order different than 1->2->3, causing broken state in your application.

If you find it hard to envision how this affects a Discord bot, it generally does not. One case I can think of is when you build "UX" using buttons and dropdowns, naively assigning event handlers to your collectors means I can break your implementation by just clicking a bunch of stuff in quick succession.

To summarize, us changing the type is not only incorrect from a typing perspective (because the return type of a callback first and firemost implies what we do with the return type, and the absence of Promise means we leave them dangling), but it's also bad in the context of this rule.

My personal recommendation is to leave it enabled globally, and whenever you find it erroring think twice if you're gonna be running state updates in the callback that depend on previous invocations. If not, disable it for that line.

[JstnMcBrd]

Fair enough. Thank you to both of you. I did manage to find another solution that resolves the lint error AND looks good. I'll put it here for anyone who finds this issue thread in the future.

// Set the client application commands and then exit the client
async function deployCommands(c: Client<true>) {
  await c.application.commands.set(commands);
  await c.destroy();
}
client.once('ready', c => void deployCommands(c));

Moving the logic into a separate method and using the void keyword explicitly ignores whether the Promise is fulfilled, which tells @typescript-eslint that it is intentional.

This way, I don't need to use eslint-disable-next-line, promise chaining, or an async IIFE method wrapper.