✨[amp-story-player] Upgrading amp-story-player to CE

[Enriqe]

Changing the player to use CustomElements. This will help us open up an API for the player.

• Also now listens to the load event listener in the window to initialize the player, instead of overriding the onload function.

Tracker #26308

To be merged after #27324

[gmajoulet]

Asking questions because I don't know much about CustomElements! :(

[Enriqe]

@jridgewell could you please help reviewing this PR, since I don't have much experience using CEs and I'm not sure if I'm doing something wrong 😄

[newmuis]

Whoops, didn't mean to approve

[Enriqe]

PTAL

[Enriqe]

Friendly ping @gmajoulet @jridgewell

[gmajoulet]

This LGTM, but I'll let @jridgewell give the approval as he understands this code a lot better than I do :)

[Enriqe]

@jridgewell friendly ping :)

[Enriqe]

bumping this up for @jridgewell

[jridgewell]

I've been OOO. Sorry for the delay.

[Enriqe]

No worries @jridgewell, thanks for taking a look!

@newmuis could you PTAL?

[newmuis]

This mostly LGTM. Approving to unblock.

Quick question @jridgewell: does Closure compiler not do anything to wrap private methods in a closure or something? Also, won't these member and function names all be minified? If so, should we keep the _ as a trailing suffix to match the Prettier rules and the rest of the repository?

[jridgewell]

does Closure compiler not do anything to wrap private methods in a closure or something?

This isn't possible for objects without considerable memory increases (every object must close over its functions and variables). This is generally frowned upon.

Also, won't these member and function names all be minified?

They should. We should check.

If so, should we keep the _ as a trailing suffix to match the Prettier rules and the rest of the repository?

Oh, in that case, yah, why not just use trailing and let the mangler make them "private"?

[Enriqe]

Reverted the commit that changed the trailing underscore to a suffix for private properties (we'll use the trailing _ like the rest of the repo), and let Closure compiler mangle the private property names (e.g. obj.foobar_ becomes obj.A). Thanks @jridgewell !

[cramforce]

Could/should we move the source into its own directory? I find it a bit weird to see it mixed with core AMP.

[Enriqe]

On the original design and at the time of launch we thought it was appropriate for it to live in the AMPHTL repo, especially since we plan on developing an AMP version of it as well. This way we could leverage the existing tools and infra of the repo, while potentially sharing tests between the two versions since the goal is to have an identical API.

Recently, it has been brought to us that users could potentially benefit from this becoming a standalone package that could be installed via npm. This would require moving this to its own repo, I have filed #27615 to track this and tackle it when appropriate.

[cramforce]

Does it really have to be its own repo? Wouldn't a directory with its own package.json work?

I was simply making a point about code-organization and not having a single mega-directory here.

[Enriqe]

Oh, my bad. Yes we should be able to move it to its own directory.