[WIP] Bento Auto-Envelope by jridgewell · Pull Request #30275 · ampproject/amphtml

jridgewell · 2020-09-17T17:02:50Z

This is a WIP PR to add Bento mode (HTML page not managed by v0 Runtime) support to our Bento components. This requires several pieces:

A minimal BaseElement implementation
A minimal CustomElement implementation to wrap the BaseElement
A "auto-detect" binary wrapper
- Which registers the Bento component with AMP Runtime if available (or we suspect it's included but not loaded)
- Or which registers the non-managed minimal implementations

amp-owners-bot · 2020-09-17T17:02:56Z

Hey @erwinmombay! These files were changed:

build-system/babel-plugins/babel-plugin-transform-amp-extension-call/index.js
build-system/babel-plugins/babel-plugin-transform-amp-extension-call/test/fixtures/transform-call/transform-simple-body/input.js
build-system/babel-plugins/babel-plugin-transform-amp-extension-call/test/fixtures/transform-call/transform-simple-body/output.js
build-system/eslint-rules/restrict-this-access.js
build-system/eslint-rules/unused-private-field.js

jridgewell · 2020-09-17T19:09:29Z

I'm currently re-using the PreactBaseElement implementation in Bento mode, but I'm not 100% sure this is the right approach. The alternative is to take the PreactBaseElement.Component and have the CustomElement directly wrap that.

I couldn't decide one way or the other. It seems like PreactBaseElement is minimal enough, and provides useful helpers for translating from the actual element in HTML to the props the Component uses. However, the PreactBaseElement may make devs too comfortable and unintentionally use AMP APIs. I tried restricting this by adding a new lint restriction on this.FOO accesses.

samouri · 2020-09-18T06:08:51Z

.eslintrc.js

+          'getProp',
+          'mutateElement',
+          'mutateProps',
+          'registerAction',


nit: do we want registerAction? my impression is that we'd only ever want the api interface

Minimally, yes, we only need to proxy api() object. But I think Justin is trying to say here that the PreactBaseElement still has to serve both worlds so moth sets of APIs are still required.

dvoytenko

This looks like taking a clear shape! I'd still like a however-minimal design doc. Some important items:

Boilerplate needs to change for Bento. Not clear how to do it "cleanly" yet. But we need to call it out in the doc as an open problem.
There are some very clear and "cheap" actions we can take now and split the work that would make this whole thing a lot easier. Namely, we just need to devirtualize CustomElement and BaseElement APIs. Anything from applyFillContent to getRealChildren can and should just become static export function somewhere.
Some of the critical decisions we'd have to make are thing like: do we have separate binaries for amp-carousel-0.1.js?bento? Or just one binary would suffice. This is something that design doc could highlight pretty well, including an estimate of the envelope's binary size.
We need to come up with an "easy" way to proxy PreactBaseElement.api() on the element.

dvoytenko · 2020-09-21T17:13:11Z

.eslintrc.js

+          'getProp',
+          'mutateElement',
+          'mutateProps',
+          'registerAction',


Minimally, yes, we only need to proxy api() object. But I think Justin is trying to say here that the PreactBaseElement still has to serve both worlds so moth sets of APIs are still required.

dvoytenko · 2020-09-21T17:17:28Z

examples/amp-bento.amp.html

+  <link rel="canonical" href="amps.html">
+  <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
+  <!-- <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> -->
+  <style amp-boilerplate>


So, this is a good point. We essentially want a different boilerplate ideally. Not sure yet how we can reconcile this with AMP mode. Ideally delta is somewhat small...

Can you explain this?

In Bento mode wouldn't we not want any boilerplate.
For regular AMP mode, why do the boilerplate requirements change?

Admittedly I don't know much about our boilerplate except that we have visibility:hidden until the page has loaded to prevent FOUC

dvoytenko · 2020-09-21T17:19:26Z

src/preact/base-element.js

      } else {
        const container = doc.createElement('i-amphtml-c');
        this.container_ = container;
-        this.applyFillContent(container);


Ok. This is a good track item: we really want to hollow out CustomEleemnt and BaseElement APIs. Sort of - everything that can become a static util - we should just make a static util, e.g. in a dom.js or a similar package.

dvoytenko · 2020-09-21T17:21:05Z

src/preact/bento-ce.js

+     * @param {function():undefined} cb
+     */
+    mutateElement(cb) {
+      Promise.resolve().then(cb);


This would probably be best to be done via setTimeout(0). That way the ordering of a mutation is still predictable. Vs a microtask ordering is completely random from the caller's point of view. Also, I think we might have more problems from overloading microtasks then tasks.

just to confirm: while this fn is necessary while we still need useResourcesNotify, ideally we can rm this fn right?

CLAassistant · 2020-10-08T23:37:17Z

All committers have signed the CLA.

samouri · 2020-10-22T18:45:34Z

build-system/compile/compile-wrappers.js

+  const payload = extensionPayload(name);
+
+  return (
+    `(function(p){self.AMP?self.AMP.push(p):` +


This is a bit tough for me to read. Is there any way for us to write this with standard-looking JS and then minify/turn into a string separately? Or at a minimum include a comment explaining what happens here.

My take:

Check if its a page with Runtime present (via v0 presence in the head or if self.AMP is already initialized)
2a. If yes, then add this extension to the AMP init array (or create it)
2b. Otherwise, define the CE immediately

Did I get that right?

samouri · 2020-10-22T18:49:50Z

examples/amp-bento.amp.html

+  <link rel="canonical" href="amps.html">
+  <meta name="viewport" content="width=device-width,minimum-scale=1,initial-scale=1">
+  <!-- <style amp-boilerplate>body{-webkit-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-moz-animation:-amp-start 8s steps(1,end) 0s 1 normal both;-ms-animation:-amp-start 8s steps(1,end) 0s 1 normal both;animation:-amp-start 8s steps(1,end) 0s 1 normal both}@-webkit-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-moz-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-ms-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@-o-keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}@keyframes -amp-start{from{visibility:hidden}to{visibility:visible}}</style><noscript><style amp-boilerplate>body{-webkit-animation:none;-moz-animation:none;-ms-animation:none;animation:none}</style></noscript> -->
+  <style amp-boilerplate>


Can you explain this?

In Bento mode wouldn't we not want any boilerplate.
For regular AMP mode, why do the boilerplate requirements change?

Admittedly I don't know much about our boilerplate except that we have visibility:hidden until the page has loaded to prevent FOUC

samouri · 2020-10-22T18:56:49Z

src/preact/bento-ce.js

+          super();
+
+          /** @const {!CeBaseElement} */
+          this.implementation = new BaseElement(this);


Should the BE implementation be initialized in the CE ctor, or in connectedCallback? I know you had intended on deferring construction to connectedCallback for AMP mode so that we can ensure the document and services are available, but updating all the unit tests was a gargantuan task

samouri · 2020-10-22T19:00:45Z

src/preact/bento-ce.js

+     * @param {function():undefined} cb
+     */
+    mutateElement(cb) {
+      Promise.resolve().then(cb);


just to confirm: while this fn is necessary while we still need useResourcesNotify, ideally we can rm this fn right?

samouri · 2020-10-22T19:06:08Z

src/preact/bento-ce.js

+if (typeof AMP !== 'undefined' && AMP.BaseElement) {
+  BaseElement = AMP.BaseElement;
+} else {
+  class CeBaseElement {


Did you experiment with trying to only have a CustomElement definition instead of a separate BaseElement as well? Do you think Bento components rely on helpers and utilities that would break without continuing the split

powerivq · 2024-09-06T11:58:28Z

Batch closing Bento PRs.

jridgewell added 5 commits September 3, 2020 00:45

tmp

6318e0f

tmp

2ecb93e

tmp

bd15320

Create CustomElement class as needed

2ac56e9

Get basic Bento mode working

b5ec8c1

jridgewell requested a review from dvoytenko September 17, 2020 17:02

google-cla bot added the cla: yes label Sep 17, 2020

samouri self-requested a review September 18, 2020 06:10

samouri reviewed Sep 21, 2020

View reviewed changes

dvoytenko reviewed Sep 21, 2020

View reviewed changes

jridgewell mentioned this pull request Sep 25, 2020

wg-runtime Status Update 2020-09-28 ampproject/wg-runtime#44

Closed

samouri reviewed Oct 22, 2020

View reviewed changes

caroqliu mentioned this pull request Jun 11, 2021

🏗 Add "wrapper": "bento" option to Bento components #34838

Merged

powerivq closed this Sep 6, 2024

Conversation

jridgewell commented Sep 17, 2020

Uh oh!

amp-owners-bot bot commented Sep 17, 2020

Uh oh!

jridgewell commented Sep 17, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CLAassistant commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

powerivq commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

CLAassistant commented Oct 8, 2020 •

edited

Loading