Skip to content

fix: update handleMessages to avoid returning true and handle promises correctly #7

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.

Sign up for GitHub

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

Draft
rugk wants to merge 1 commit into
base: main
Choose a base branch
from
Draft

fix: update handleMessages to avoid returning true and handle promises correctly #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 8 additions & 23 deletions BrowserCommunication.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,40 +75,25 @@ function handleMessages(request, sender, sendResponse) {

if (!(messageType in callbacks) || callbacks[messageType].length === 0) {
console.warn(`No callbacks for message type "${messageType}" registered.`);
return true;
return;
}

// call all callbacks and keep return values
const promises = [];
let gotTrueAsReturn = false;
for (const callback of callbacks[messageType]) {
const returnValue = callback(request, sender, sendResponse);

// notice if return value is just "true"
if (returnValue === true) {
gotTrueAsReturn = true;
continue;
// If the callback returns a Promise, add it to the list
if (returnValue && typeof returnValue.then === "function") {
Copy link
Member Author

@rugk rugk Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking promises with returnValue.then === "function" looks fishy/hacky to me from the first loook…

Copy link

@tdulcet tdulcet Aug 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@rugk rugk Sep 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

promises.push(returnValue);
}

// return value should be a Promise
promises.push(returnValue);
}

// handle returning
if (gotTrueAsReturn) {
Copy link
Member Author

@rugk rugk Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/runtime/onMessage#Parameters

Ah and yeah that could be it: the new code totally destroys the special handling of true for async stuff.

if (callbacks[messageType].length !== 1) {
// if it was not the only callback, then show a real error
console.error(`At least one callback for message type "${messageType}" returned the legacy value "true".
As you have registered ${callbacks[messageType].length} listeners this may lead to errors.`);
} else {
// show warning as this behaviour is discouraged
console.warn(`At least one callback for message type "${messageType}" returned the legacy value "true". Please return a Promise instead.`);
}

return true;
if (promises.length > 0) {
// If there are async handlers, return a Promise
return Promise.all(promises);
}

return Promise.all(promises);
// Otherwise, return nothing (void)
}

/**
Expand Down