Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow listeners passed into runtime.onMessage.addListener to return void #109

Open
namukang opened this issue Nov 24, 2024 · 4 comments
Open

Comments

@namukang
Copy link

Previously, listeners for runtime.onMessage.addListener could return Promise<unknown> | true | void:

) => Promise<unknown> | true | void

Currently, they require a return value of Promise<unknown> | true | undefined:

) => Promise<unknown> | true | undefined

Commit where change was made: 85437fc

I believe the correct type should be void rather than undefined so that listeners can return without having to specify undefined.

@Lusito
Copy link
Owner

Lusito commented Feb 14, 2025

The people at DefinitelyTyped are a bit hard on disabling their linting rules globally, so this was the easy fix.

Can you show me an example where this is an issue for you? I don't believe you need to specify undefined for it to work.

@Lusito
Copy link
Owner

Lusito commented Feb 22, 2025

For reference, this is where the discussion at DT happened: DefinitelyTyped/DefinitelyTyped#69704
And this is the issue from the linting rule, still in discussion: typescript-eslint/typescript-eslint#5752

So there are 3 ways to solve this:

  1. convince DT maintainers to allow a global disable of the rule.
  2. rewrite the generator code, so that it adds a rule-disable comment per line (won't be easy)
  3. convince the maintainers of typescript-eslint to make this case allowed in their rule and wait for DT to update their dependency on it.

@namukang
Copy link
Author

namukang commented Feb 22, 2025

Sure, here's an example:

Here's a minimal message listener that logs all received messages:

browser.runtime.onMessage.addListener((message) => {
  console.log(message);
});

With the updated type definition for runtime.onMessage.addListener, the above results in the following type error:

Argument of type '(message: unknown) => void' is not assignable to parameter of type '(message: unknown, sender: MessageSender, sendResponse: (message: unknown) => void) => true | Promise<unknown> | undefined'.
  Type 'void' is not assignable to type 'true | Promise<unknown> | undefined'.ts(2345)

As a result, the message listener needs to include an extra return undefined to resolve it:

browser.runtime.onMessage.addListener((message) => {
  console.log(message);
  return undefined;
});

I was hoping that reverting the type definition would be a quick fix, but given the issues you referenced above, I understand the decision you made and agree that it seems like a lot of effort given the upstream challenges.

Thanks for following up with me and sharing the context behind it! (Also, as someone who loves type-safety and has been using this library since it was webextension-polyfill-ts, thank you so much for all your work and effort! <3)

@Lusito
Copy link
Owner

Lusito commented Feb 23, 2025

I might have found a solution, that even improves types to only allow sendResponse with return true. Let's see if this gets approved: DefinitelyTyped/DefinitelyTyped#72037

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants