-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
🐛 Change to using setTimeout to avoid muted unhandledrejections #25342
Conversation
It also directly calls into the globally installed error reporting function. Re: ampproject#25289 It seems HTML spec is unlikely to change: whatwg/html#5051
@@ -106,7 +106,8 @@ function isPatched(win) { | |||
* @param {!Error} error | |||
*/ | |||
function rethrowAsync(error) { | |||
new /*OK*/ Promise(() => { | |||
setTimeout(() => { | |||
self.__AMP_REPORT_ERROR(error); |
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.
Do we know for sure that setReportError()
will be called by this point?
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.
Yes, this can't happen until an AMP element has been created, which happens after the log reporter is setup.
Hm, this might be causing local and PR test failures for me:
|
16bf905 seems to fix it. |
*fixed one instance. Tracking down these async Can't cleanly check for "test mode" here, so maybe we can store these errors somewhere and throw them periodically instead? |
Merged master and the test failures are no longer happening on my PR #25066. 🤷♂ |
…ject#25342) It also directly calls into the globally installed error reporting function. Re: ampproject#25289 It seems HTML spec is unlikely to change: whatwg/html#5051
It also directly calls into the globally installed error reporting function.
Re: #25289
It seems HTML spec is unlikely to change: whatwg/html#5051