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

window.reportError(e)? #13484

Closed
benjamingr opened this issue Jan 25, 2022 · 10 comments · Fixed by #13799
Closed

window.reportError(e)? #13484

benjamingr opened this issue Jan 25, 2022 · 10 comments · Fixed by #13799
Labels
feat new feature (which has been agreed to/accepted) help wanted community help requested web related to Web APIs

Comments

@benjamingr
Copy link
Contributor

The web platform supports self.reportError(e) to trigger an uncaught error with semantics similar to setTimeout(() => { throw e; }).

Would it make sense for Deno to implement this API?

@littledivy littledivy added suggestion suggestions for new features (yet to be agreed) web related to Web APIs labels Jan 25, 2022
@bartlomieju
Copy link
Member

Looking at mdn it appears that this API is only available in Worker context.

@andreubotella
Copy link
Contributor

It isn't. It's also available in workers.

@benjamingr
Copy link
Contributor Author

Yes if you look at the spec it's exposed in both cases.

(I am not sure if adding it is the thing to do - we decided not to implement this API (yet) in Node.js and the suggestion to add it stalled)

@bartlomieju
Copy link
Member

Okay, this seems useful. I'm in favor of adding this API. If anyone is interested in working on a PR, let me know, we'll try to provide some pointers.

@bartlomieju bartlomieju added feat new feature (which has been agreed to/accepted) help wanted community help requested and removed suggestion suggestions for new features (yet to be agreed) labels Feb 15, 2022
@nayeemrmn
Copy link
Collaborator

I've been working on this. It seems everything reportError() does is synchronous based the spec and browser behaviour, contrary to the setTimeout() example. Otherwise the behaviour shared with setTimeout() boils down to:

  • An error event is dispatched to the global scope.
  • The error is printed to the console in the format of an exception.
  • In server side environments, additionally the process will be terminated if the error isn't intercepted by a handler:
    // Node
    process.on("uncaughtException", (e) => {});
    
    // Deno
    addEventListener("error" (e) => {
      e.preventDefault();
    });

@benjamingr Do I have this correct with respect to what the Node behaviour would be + process termination?

@benjamingr
Copy link
Contributor Author

@benjamingr Do I have this correct with respect to what the Node behaviour would be + process termination?

You do, though I still haven't merged the Node.js PR since I am still not 100% sure this is a good API.

Also "The error is printed to the console in the format of an exception." - this likely won't happen if there is an errorMonitor or uncaughtException handler so the user can redirect these to monitoring.

@nayeemrmn
Copy link
Collaborator

Also "The error is printed to the console in the format of an exception." - this likely won't happen if there is an errorMonitor or uncaughtException handler so the user can redirect these to monitoring.

Perfect, yeah I meant that to be conditioned under the unhandled case as well since browsers already do that with e.preventDefault().

Looking at nodejs/node#41912 it looks like we reached the same conclusion to crash by throwing on the next tick. One difference however is that by elevating the error for the first time at the next tick means it won't be printed synchronously like it is in browsers.

I avoided this by splitting the error into two, the input error is printed immediately while a generic error is thrown next tick to cause the crash:
console.log(1);
reportError(new Error("foo"));
console.log(2);

image

Probably wouldn't be as palatable if we didn't already have this kind of formatting for child worker errors:
// temp.ts
new Worker(new URL("temp2.ts", import.meta.url).href, { type: "module" });

// temp2.ts
throw new Error("foo");

image

Ref https://github.com/denoland/deno/blob/3dec3301108a3dc85ffd99c5e60eb5ab613150b0/ext/web/16_report_error.js

@benjamingr
Copy link
Contributor Author

What do you think should happen in terms of developer experience when someone does reportError("error") (that is - something without a stack trace)?

We are discussing either warning in Node.js (a non-error causes a warning with a stack trace), always logging the stack trace when .reportError is called at the call site or doing nothing.

@nayeemrmn
Copy link
Collaborator

@benjamingr Maybe Node should tie it to --trace-uncaught and essentially just match the output of throw "error".

$ node -e 'reportError("error")'
[eval]:1
reportError("error")
^
1
(Use `node --trace-uncaught ...` to show where `reportError()` was called)

Node.js vX.X.X

$ node --trace-uncaught -e 'reportError("error")'
[eval]:1
reportError("error")
^
1
`reportError()` called at:
    at [eval]:1:1
    at runInThisContext (node:vm:129:12)
    at runInThisContext (node:vm:305:38)
    at node:internal/process/execution:76:19
    at [eval]-wrapper:6:22
    at evalScript (node:internal/process/execution:75:60)
    at node:internal/main/eval_string:27:3

Node.js vX.X.X

@lucacasonato
Copy link
Member

Will be released in Deno 1.21.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat new feature (which has been agreed to/accepted) help wanted community help requested web related to Web APIs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants