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

amp-script: catch user errors in worker scripts #28207

Merged
merged 2 commits into from May 6, 2020

Conversation

samouri
Copy link
Member

@samouri samouri commented May 5, 2020

summary
Fixes #28198.

Adds an onerror handler to the worker script so that we can rethrow all errors as user errors instead of bubbling into a dev one. This should help clear some noise from the error tracker.

Here's a screenshot of the new error format:
Screen Shot 2020-05-05 at 3 10 53 PM

cc @rcebulko @ampproject/wg-runtime

Copy link
Contributor

@rcebulko rcebulko left a comment

Choose a reason for hiding this comment

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

Thanks for taking care of this!

@@ -247,6 +247,13 @@ export class AmpScript extends AMP.BaseElement {
config
).then((workerDom) => {
this.workerDom_ = workerDom;
this.workerDom_.onerror = (errorEvent) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is workerDom a Worker instance?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep. its...a worker from worker-dom. It would potentially make sense to just name it worker. This is the exact API I plan on changing in ampproject/worker-dom#850

@samouri samouri merged commit 5a041f1 into ampproject:master May 6, 2020
@samouri samouri deleted the script-user-error branch May 6, 2020 00:46
ldoroshe pushed a commit to ldoroshe/amphtml that referenced this pull request May 8, 2020
* amp-script: catch user errors in worker scripts

* remove obnoxious laughs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Catch errors in publisher amp-script code
6 participants