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: improved error messages #29841
Conversation
553e3bf
to
e357130
Compare
} | ||
}) | ||
.catch((err) => { | ||
// Catch errors if due to an issue with workerAndAuthorScripts rejection. |
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.
Can we just chain a catch()
onto workerAndAuthorScripts
above?
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.
WorkerDOM.upgrade
returns a promise which is the uncaught one, so chaining onto workerAndAuthorScripts
won't help.
The more obvious thing to do IMO would be to return the WorkerDOM.upgrade promise instead of workerAndAuthorScripts
down below. Current method seems very intentional though, is there a reason why we don't do it?
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.
Oh let's just do that. Don't think so.
* amp-script: improved error messages * remove double error caused by WorkerDOM.upgrade * need an extra space * lint * eh, back to createError * less brittle check for script failure * keep nonnull return promise * cleanest way yay * "or incorrect?" * fix test
Fixes #29614
For the reviewer: adding a
.catch
toWorkerDOM.upgrade
was hacky. If an error occurs within the worker-dom code, we'd probably want an uncaught exception to throw, whereas if it is caused by the code to retrieve authorScript, then we don't want that.The cleanest way of having the same effect would be to have the entire upgrade path within a
.then()
callback of the scripts, although that would then delay the start of the upgrade until after they've resolved (not that I think worker-dom actually does much work before they do, but it is what caused my hesitation).