🐛 Add error messages to thrown bare Error()s in amp-script

[rcebulko]

As part of #24266, error messages with substitutions aren't logged properly. Some parts of amp-script use user().error() to log locally, then throw new Error(), resulting in AMP Error Reporting getting a smattering of bare Error: Error reports, which add to error reporting noise and are difficult for a release on-duty to assess/debug

This PR updates those bare throws to instead throw a user error with a useful message.

Closes #28184

[dreamofabear]

Does this result in two console messages for a single error? Might be better to catch these errors which would prevent them from showing up in the error reporter due to unhandled rejection.

[rcebulko]

Does this result in two console messages for a single error?

Indeed it does! Currently the page logs one useful/informative error, followed by two completely empty errors:

These changes would fix the blank error messages to be more informative, making it clear the error is the same.

Might be better to catch these errors which would prevent them from showing up in the error reporter due to unhandled rejection.

The first user.error() logs to the console, but doesn't actually throw out of the method. There are no catch blocks in amp-script.js, which led me to believe the intention may be for publishers working with amp-script to fail fast on errors/misconfigurations, and it makes sense for errors like this to throw/escalate.

More generally, I definitely agree errors in publishers' custom amp-script code should be caught and not logged to error reporting, but that felt out-of-scope for this PR.

I imagine you'd prefer to see the underlying issues here (#24266 and #27904) actually fixed, which this PR does not do. My objective with this PR was more of a small step forward, adding more information to both the console and error reporting, while the underlying issues are explored and addressed

Edit: Filed new issue #28198 for tracking handling of amp-script errors in publisher code

[dreamofabear]

So I think the error string extraction hasn't launched yet anyways, so we can ignore the "use %s" guidance for now and just use JS template literals. I.e. make the new thrown user().createError match the old error exactly with JS, and remove the old user().error. And update the TODO to "Refactor to %s interpolation when error string extraction is ready.".

Thanks for fixing this.

/cc @ampproject/wg-performance

[rcebulko]

I.e. make the new thrown user().createError match the old error exactly with JS, and remove the old user().error. And update the TODO to "Refactor to %s interpolation when error string extraction is ready.".

SGTM!

[rcebulko]

/cc @rsimha
It appears the Codecov check is testing the latest commit on this PR with one from around 4 days ago. The code added since then means that the diff doesn't meet the threshold and this PR is blocked. Is something going awry here?

[rsimha]

Is something going awry here?

@rcebulko, thanks for alerting me. Codecov is choosing a base commit that's 4 days old because your branch was forked from master 4 days ago. It doesn't seem to be accounting for all the new commits to master since then, which are in fact included in the code built by Travis.

https://travis-ci.org/github/ampproject/amphtml/jobs/683465296

I'll look into manually setting the base commit to master. Will continue this discussion in #28212. Meanwhile, the good news is that the codecov check is non-required and therefore non-blocking, even though it is a red X mark.