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

Where are notification errors handled? #40

Open
fregante opened this issue Oct 13, 2021 · 2 comments
Open

Where are notification errors handled? #40

fregante opened this issue Oct 13, 2021 · 2 comments

Comments

@fregante
Copy link
Collaborator

fregante commented Oct 13, 2021

Notifications never throw errors, they are not meant to be awaited and therefore there's no way to async catch errors either.

I think that currently every notification errors are just logged to the console, but they don't bubble up and reportError won't catch them.

webext-messenger/index.ts

Lines 147 to 149 in 2e13ca0

void sendMessage().catch((error: unknown) => {
console.debug("Messenger:", type, "notification failed", { error });
});

Question

  • Should such error be re-thrown in that handler? That would let a global unhandledrejection handler catch it.

Alternatively, a user interested in errors should just not send notifications so that they can use the usual:

myApiCall().catch(reportError)

Prior art

A background forwarder catches an error:

https://github.com/pixiebrix/pixiebrix-extension/blob/ea335a13e28a8c63ab1c73a6e4c2c2ba27a65da9/src/background/browserAction.ts#L250-L252

But the original sender just ignores the message:

https://github.com/pixiebrix/pixiebrix-extension/blob/ea335a13e28a8c63ab1c73a6e4c2c2ba27a65da9/src/actionPanel/native.tsx#L167-L168

With integrated message forwarding (#23, #31) this user-defined middleman/handler won't exist.

@twschiller
Copy link
Collaborator

twschiller commented Oct 18, 2021

I'm not sure I'm 100% clear when you say it "handler" refers to:

  1. The custom handler logic
  2. The messaging framework listener + the custom handler logic

Is handler and listener the precise terms?

Philosophically, if the caller is sending a "notification" and therefore doesn't care about the fulfilled response, they shouldn't receive the error messages thrown by the custom handler

If the an error is thrown in the handler, my vote would be to let the unhandledrejection handler handle it. Or, to allow the context to register a method that listens for handler errors

@fregante
Copy link
Collaborator Author

I'm not sure I'm 100% clear when you say it "handler" refers to:

In my question I refer specifically to the error handler. By re-throwing I mean:

 void sendMessage().catch((error: unknown) => { 
   const communicationError = new CommunicationError(type + " notification failed");
   communicationError.cause = error;
   throw communicationError;
 });

Is handler and listener the precise terms?

I rarely use listener, but it sounds interchangeable with handler if we're referring to functions that are called by event emitters.

If the an error is thrown in the handler, my vote would be to let the unhandledrejection handler handle it. Or, to allow the context to register a method that listens for handler errors

Great, so I'll use the pattern I just suggested.

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

No branches or pull requests

2 participants