Amphtml: 🌸 Cherry-pick request for #27732 into #27625/#27765 (Approved)

[Enriqe]

Cherry-pick request

| Issue | PR | Beta / Experimental? | Stable? | LTS? | Release issue |
| :---------------: | :------------: | :------------------: | :----------: | :----------: | ------------------------------------------------------------------------------- |
| #27732 | #27733 | YES | YES | NO | Stable: #27625 \| Beta/Experimental: #27765 |

Why does this issue meet the cherry-pick criteria?

When opted-in to beta channel, amp-story-player fails to load completely.

Mini-postmortem

We upgraded the player to extend from the native HTMLElement class, and since the code is transpilled from a JS version that supports class syntax to one that doesn't, it is not possible to use CustomElements without a polyfill.

Since the code in the local server is not the compiled version, I didn't catch this error when running the code locally. And since we normally ship a polyfill with v0.js, but we don't expect v0.js to be loaded in the player context, this wasn't in our top of mind.

What could've been done to prevent this?

Since this was a transpillation issue, I should've tested the compiled code.

Integration tests use compiled code, but they also install the polyfill, so they wouldn't have caught this. A simple sanity check using the --compiled flag with a clean page that doesn't load v0.js (since it also installs the polyfill) would have caught this.

Where we got lucky

The report came from a user using the beta opt-in, so we were able to detect and fix the issue before it went into stable.

Summary

Because the code shipped is transpilled from a class syntax to a function syntax, it can't extend the native HTMLElement class, which is something CEv1 does. Since we don't ship a polyfill or expect publishers to install a CEv1 polyfill themselves, the code fails to run.

Action Items

• Add e2e / integration tests that's representative of real world usage #27827

---

/cc @ampproject/wg-approvers @ampproject/cherry-pick-approvers

[Enriqe]

@dvoytenko @cramforce

[Enriqe]

Fix is a clean rollback

[cramforce]

Clean rollbacks to beta do not need approval.

[danielrozenberg]

Generally speaking since we switch to nightly builds, we should be promoting a build from earlier in the week (Incident Resolution section in #25616) - the offending PR was included in Tuesday's release, so ideally I would have just promoted Monday's release... unfortunately Monday's release didn't go through, so I _do_ have to perform this cherry-pick 😤

[Enriqe]

Ack, sorry for the troubles @danielrozenberg and thanks for handling this!

[danielrozenberg]

(changed the related release from #27733 to #27765 as this affects the _new_ Beta/Experimental releases)

[Enriqe]

Finished writing mini PM, for whoever is interested.

[danielrozenberg]

Performed the cherry-pick and promoted it to opt-in

[danielrozenberg]

@dvoytenko @cramforce - turns out we _do_ need to cherry-pick this onto Stable, so I'm reopening this to get approval again.

Here's what happened:

When @Enriqe created this cherry-pick request, 2004030010070 was the Stable version, and that one didn't include the offending PR.

But 2004030010070 is the release that was created by doing a revert on a separate offending PR, but directly by reverting on the release branch, not by cherry picking a revert PR (our process specifically asks to only ever create releases from commits on master or from branches that have commits that were cherry-picked from master. @dvoytenko created FR #27771 to prevent this from happening again)

The next day I promoted 2004071640410. This release includes a master commit that reverts the offending PR for the issue that necessitated creating 2004030010070, but now also includes the offending PR that this cherry-pick asked to revert (that came the next day)

From 2004071640410, all new Stable releases were created from cherry-picks on top of that one

Basically we stumbled on a release race condition :(

[dvoytenko]

Approved

[choumx]

Thanks for filling in the mini-PM. @Enriqe QQ about this bit:

Integration tests use compiled code, but they also install the polyfill, so they wouldn't have caught this. A simple sanity check using the --compiled flag with a clean page that doesn't load v0.js (since it also installs the polyfill) would have caught this.

Do we have an action item to add an e2e or integration test that's representative of real usage? It should be possible to avoid the polyfill (not include v0.js) in a test fixture.

[Enriqe]

Good point @choumx, since we're not pursuing the CE route anymore we shouldn't run into this type of issue again. I still agree it's a good idea to add some e2e / integration tests, so I filed #27827.