-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Introduce self.reportError() #1196
Conversation
So based on following the links it looks like @bzbarsky is in favor of implementing this in Gecko. Any other implementers interested? |
Will the be hooks for scripts to hook into this (ie, for New Relic or Sentry error reporting?), or will they still have to trampoline through |
What do you mean by "trampoline through |
I didn't know that |
source
Outdated
<div w-nodev> | ||
|
||
<p>The <dfn data-x="dom-reportException"><code>reportException(<var>exception</var>)</code></dfn> | ||
method, when invoked, must <span>report an exception</span> with <var>exception</var>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this do nothing if it's called inside window.onerror
(like runtime script errors in onerror
are no-ops)? (Can still report something to devtools though.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does nothing, right? Target would be in error reporting mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, sorry. I misremembered the logic.
To be clear, the reason we want this is for JavaScript to be able to emulate the behavior of |
It seems this is still the right approach even after "report an exception" is fixed in #958. We don't handle muting here, but I don't think we need to. |
Here's one feedback: have we considered adding this on |
@rniwa we did; that was actually the original suggestion. However, it fires an |
Surely Blink's "explain the platform" folks are interested in this? |
97f6c39
to
2ed245b
Compare
@rniwa any more thoughts? Shall we do this? @mathiasbynens perhaps you can figure out Chrome support for this? |
@rniwa @mathiasbynens ping. |
@clelland can you take a look at this for Chrome? I suspect there is a connection to https://w3c.github.io/reporting/ in that things which are reported through |
I found https://github.com/ReactiveX/rxjs/blob/master/src/internal/util/reportUnhandledError.ts showing how at least one popular library is doing this today: indeed, it's with https://github.com/kriskowal/asap seems to do something similar, except hidden behind a few layers of abstraction. Any thoughts, @kriskowal? |
Not sure if this is the same thing as you're asking, but we use this trick in React: Essentially, when we catch a user error in our Our trick is to call user code inside the context of a fake DOM event dispatch, and listen to the |
ASAP would certainly have benefited from a mechanism to surface an error as if it had been thrown but without throwing. Much of the complexity in ASAP flows from preserving execution order and amortizing timers. Any time it needs to report an error, ASAP stops flushing its event queue, schedules a flush, and throws the error. With, |
@domenic Absolutely! Whenever we implemented this, I was honestly curious why something like that didn't already exist. Although, we would probably still end up using |
755c57a
to
0dae6cc
Compare
Opened a discussion in Node.js :) nodejs/node#38947 |
This seems like something we should also expose in worklets, right? cc @padenot |
Since nobody has had further strong opinions on the name I'm going to change it to reportError(). |
Each worklet implement their own error reporting, if any, and for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot approve my own PR, but I still think this is okay. I'll update OP with the PR template as I don't think we have tests for this yet.
While writing tests I briefly wondered if we wanted to expose whether the exception was handled in some way, but I could not think of a reason why a library would want control over that. Especially as the use case is emulating what happens if an exception is thrown in certain contexts. (If an exception is not handled, the browser will report it in the console.) |
The tests made me realize we probably want |
Oh, hmm, re-reading https://heycam.github.io/webidl/#es-any it looks like I guess the difference between |
Automatic update from web-platform-tests HTML: self.reportError() For whatwg/html#1196. -- wpt-commits: c3cb1109a30e82ceccdf4202d26f6c6648bbef70 wpt-pr: 29738
Automatic update from web-platform-tests HTML: self.reportError() For whatwg/html#1196. -- wpt-commits: c3cb1109a30e82ceccdf4202d26f6c6648bbef70 wpt-pr: 29738
Automatic update from web-platform-tests HTML: self.reportError() For whatwg/html#1196. -- wpt-commits: c3cb1109a30e82ceccdf4202d26f6c6648bbef70 wpt-pr: 29738
Automatic update from web-platform-tests HTML: self.reportError() For whatwg/html#1196. -- wpt-commits: c3cb1109a30e82ceccdf4202d26f6c6648bbef70 wpt-pr: 29738
https://bugs.webkit.org/show_bug.cgi?id=228316 <rdar://problem/81446162> Reviewed by Sam Weinig. LayoutTests/imported/w3c: Import test coverage from: - web-platform-tests/wpt#29738 * web-platform-tests/html/webappapis/scripting/reporterror.any-expected.txt: Added. * web-platform-tests/html/webappapis/scripting/reporterror.any.html: Added. * web-platform-tests/html/webappapis/scripting/reporterror.any.js: Added. (undefined.forEach.throwable.test.t.assert_equals): (test): * web-platform-tests/html/webappapis/scripting/reporterror.any.worker-expected.txt: Added. * web-platform-tests/html/webappapis/scripting/reporterror.any.worker.html: Added. * web-platform-tests/html/webappapis/scripting/w3c-import.log: Added. Source/WebCore: Implement self.reportError() as per: - whatwg/html#1196 Firefox already shipped this and Chrome will do so soon too. Tests: imported/w3c/web-platform-tests/html/webappapis/scripting/reporterror.any.html imported/w3c/web-platform-tests/html/webappapis/scripting/reporterror.any.worker.html * page/DOMWindow.cpp: (WebCore::DOMWindow::reportError): * page/DOMWindow.h: * page/WindowOrWorkerGlobalScope.idl: * workers/WorkerGlobalScope.cpp: (WebCore::WorkerGlobalScope::reportError): * workers/WorkerGlobalScope.h: Canonical link: https://commits.webkit.org/241098@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@281756 268f45cc-cd09-0410-ab3c-d52691b4dbfc
https://bugs.webkit.org/show_bug.cgi?id=228316 <rdar://problem/81446162> Reviewed by Sam Weinig. LayoutTests/imported/w3c: Import test coverage from: - web-platform-tests/wpt#29738 * web-platform-tests/html/webappapis/scripting/reporterror.any-expected.txt: Added. * web-platform-tests/html/webappapis/scripting/reporterror.any.html: Added. * web-platform-tests/html/webappapis/scripting/reporterror.any.js: Added. (undefined.forEach.throwable.test.t.assert_equals): (test): * web-platform-tests/html/webappapis/scripting/reporterror.any.worker-expected.txt: Added. * web-platform-tests/html/webappapis/scripting/reporterror.any.worker.html: Added. * web-platform-tests/html/webappapis/scripting/w3c-import.log: Added. Source/WebCore: Implement self.reportError() as per: - whatwg/html#1196 Firefox already shipped this and Chrome will do so soon too. Tests: imported/w3c/web-platform-tests/html/webappapis/scripting/reporterror.any.html imported/w3c/web-platform-tests/html/webappapis/scripting/reporterror.any.worker.html * page/DOMWindow.cpp: (WebCore::DOMWindow::reportError): * page/DOMWindow.h: * page/WindowOrWorkerGlobalScope.idl: * workers/WorkerGlobalScope.cpp: (WebCore::WorkerGlobalScope::reportError): * workers/WorkerGlobalScope.h: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@281756 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Fixes whatwg/console#50.
(See WHATWG Working Mode: Changes for more details.)
/webappapis.html ( diff )