fix: update handleMessages to avoid returning true and handle promises correctly #7
Conversation
|
Maybe you can give your opinion here, @tdulcet? I need a second pair of eyes… |
| gotTrueAsReturn = true; | ||
| continue; | ||
| // If the callback returns a Promise, add it to the list | ||
| if (returnValue && typeof returnValue.then === "function") { |
There was a problem hiding this comment.
Checking promises with returnValue.then === "function" looks fishy/hacky to me from the first loook…
There was a problem hiding this comment.
If you are OK removing the special handing for true, the function body could be simplified to just:
return Promise.all(callbacks[messageType].map((callback) => callback(request, sender, sendResponse)));Promise.all already supports empty and non-promise values.
There was a problem hiding this comment.
Hmm AFAIK the special handling of true is unfortunately not something I invented, see:
return true from the event listener. This keeps the sendResponse() function valid after the listener returns, so you can call it later. See the sending an asynchronous response using sendResponse example.
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/onMessage
(Ah already quoted, so this just means I cannot change this/somehow need to stay compatible, I guess.)
| } | ||
|
|
||
| // handle returning | ||
| if (gotTrueAsReturn) { |
There was a problem hiding this comment.
Though I agree that this workaround was ugly too… 😅
This is likely base don this MDN advise:
return true from the event listener. This keeps the sendResponse() function valid after the listener returns, so you can call it later. See the sending an asynchronous response using sendResponse example.
Ah and yeah that could be it: the new code totally destroys the special handling of true for async stuff.
AI-generated by Copilot, so I do not fully trust this.
It is based on #6 and it "fixed" this while wanting to fix the return type typing issue here (f6e03a6)