Setting up code for AmpStoryEmbed viewer #26276
Conversation
| @@ -144,13 +144,13 @@ exports.jsBundles = { | |||
| includePolyfills: false, | |||
| }, | |||
| }, | |||
| 'amp-story-embed.js': { | |||
| 'amp-story-embed-manager.js': { | |||
There was a problem hiding this comment.
What's motivating the name change? This is still the JavaScript file for the <amp-story-embed> component. This new name implies that it manages amp-story-embeds, which it does not, except for the few lines of code at the end.
There was a problem hiding this comment.
Yeah, this change I think made sense in the context of #25006, but not here
There was a problem hiding this comment.
I thought we might want to keep it for when we introduce the AMP extension, to differentiate it and avoid renaming the import script?
There was a problem hiding this comment.
When publishers use this, do we want them to import
https://cdn.ampproject.org/v0/amp-story-embed-0.1.js for the AMP version
and https://cdn.ampproject.org/v0/amp-story-embed-manager-0.1.js for the pure-JS version?
There was a problem hiding this comment.
What I'm going to is.. if they are both called amp-story-embed.js won't they override each other under https://cdn.ampproject.org/v0/? or will they be in separate directories?
There was a problem hiding this comment.
Isn't you last bit of code using if (!isAMP) meant to keep the same API so the same script works on both AMP and non AMP?
Even without this, the file name should reflect the name of the class it contains. And here, the class should really be AmpStoryEmbed
There was a problem hiding this comment.
Agreed with Gabriel here.
I think we should just drop the AMP/non-AMP split entirely and not implement this until we have an AMP version in hand.
What I'm going to is.. if they are both called amp-story-embed.js won't they override each other under https://cdn.ampproject.org/v0/? or will they be in separate directories?
They're different URLs. This is https://cdn.ampproject.org/amp-story-embed-v0.js, whereas the AMP extension would be https://cdn.ampproject.org/v0/amp-story-embed-0.1.js. At some point, if we have a binary that serves a version that can serve both AMP and non-AMP, we can alias these URLs to that binary
There was a problem hiding this comment.
Ah nice, thanks for clarifying
|
/to @jridgewell for OWNERS approval |
|
Good question, I'll send a PR. Thanks all for the review :) |
Issue tracker #26308
Partial for #24539
Picking up work from #25006 by @newmuis
In preparation of having a pure JS library and an AMP extension of the viewer, this PR sets up the structure for managing the logic of both.
Given that our priority is the JS library for non-AMP pages, let's focus on this one first and then create the AMP extension which will use this new class.