Introduce ampdoc on a FIE level#22493
Conversation
dreamofabear
left a comment
There was a problem hiding this comment.
Also would be great to remove all of the "EmbedScope" service getters in service.js and element-service.js.
| // Order is important! | ||
|
|
||
| // QQQQQQ: all service passthrough to embed are bad here. some are near-critical. | ||
| // remove them asap. |
There was a problem hiding this comment.
Isn't status quo that all services pass-through/inherit except the following?
url, actions, standard-actions, navigation, timer, bind
Seems like we should keep that behavior at least for MVP migration.
There was a problem hiding this comment.
It's actually not, no. You're right that we may hit some compatibility blocks and for that reason I will have passthrough under experiment. But the final state: no passthrough. In very rare cases where passthrough is needed we just need to do it explicitly.
There was a problem hiding this comment.
"No pass-through" sounds kind of heavy. Are you thinking this is better for security?
Also, does it prevent lightweight, "root" use cases?
There was a problem hiding this comment.
I think we definitely need "open" and "close" modes. And I'm pretty sure they will be easier to accomplish for us. But I want to focus on "close" first since that covers our most current cases. "Open" roots would take some more discussion to figure out.
| getAmpdocServiceHolder(ampdoc.getEmbedder()), id); | ||
| registerServiceInternal( | ||
| getAmpdocServiceHolder(ampdoc), ampdoc, id, () => service); | ||
| } |
There was a problem hiding this comment.
I want to eventually remove most of extensions of AmpDoc and have a single AmpDoc with few customizations. Basically everyone to go into reducing differences between ampdocs.
Can we get rid of this if we allow configuration of AmpDoc for FIE such that it inherits all services except for the ~5-6 that matter?
There was a problem hiding this comment.
That's definitely the "final state" plan for this migration. You can see in the prototype that there's only a single "install root" method and everything else is essentially the same with vary small customizations.
|
All of this code, except for cleanup, has been merged in a series of many pull requests. See #22734 for more info. |
Superceded by #22734.
First I wanted to note - this is a fairly drastic PR. I'm not planning to merge it as is, but instead do these migrations in phases. Sometimes even guarded by an experiment.
The key changes here:
AmpDocand have a singleAmpDocwith few customizations. Basically everyone to go into reducing differences between ampdocs.