Deferred build API and builder (#32568)

* Deferred build API and builder * review fixes * Ensure to rebuild on re-attach * Rename V2 to V1 * review fixes * lints * onReadyState to setReadyState * review fixes * notes * fixed tests * whenBuild -> build * cleanup tests * lints * fix form/sticky-ad tests * debugging tests * debugging 2 * debugging 3 * debugging tests 3
ampproject · Feb 25, 2021 · 46f2dd0 · 46f2dd0
1 parent 9ebd283
commit 46f2dd0
Show file tree

Hide file tree

Showing 28 changed files with 3,078 additions and 425 deletions.
diff --git a/build-system/global-configs/experiments-const.json b/build-system/global-configs/experiments-const.json
@@ -1,5 +1,6 @@
 {
   "BENTO_AUTO_UPGRADE": false,
   "INI_LOAD_INOB": false,
+  "V1_IMG_DEFERRED_BUILD": false,
   "WITHIN_VIEWPORT_INOB": false
 }
diff --git a/build-system/tasks/presubmit-checks.js b/build-system/tasks/presubmit-checks.js
@@ -147,6 +147,18 @@ const forbiddenTerms = {
       'extensions/amp-analytics/0.1/requests.js',
     ],
   },
+  '\\.buildInternal': {
+    message: 'can only be called by the framework',
+    allowlist: [
+      'src/service/builder.js',
+      'src/service/resource.js',
+      'testing/iframe.js',
+    ],
+  },
+  'getBuilderForDoc': {
+    message: 'can only be used by the runtime',
+    allowlist: ['src/custom-element.js', 'src/service/builder.js'],
+  },
   // Service factories that should only be installed once.
   'installActionServiceForDoc': {
     message: privateServiceFactory,
@@ -310,6 +322,7 @@ const forbiddenTerms = {
       'src/chunk.js',
       'src/element-service.js',
       'src/service.js',
+      'src/service/builder.js',
       'src/service/cid-impl.js',
       'src/service/origin-experiments-impl.js',
       'src/service/template-impl.js',
@@ -1387,6 +1400,7 @@ function hasAnyTerms(srcFile) {
     /^test-/.test(basename) ||
     /^_init_tests/.test(basename) ||
     /_test\.js$/.test(basename) ||
+    /testing\//.test(srcFile) ||
     /storybook\/[^/]+\.js$/.test(srcFile);
   if (!isTestFile) {
     hasSrcInclusiveTerms = matchTerms(srcFile, forbiddenTermsSrcInclusive);

diff --git a/builtins/amp-img.js b/builtins/amp-img.js
@@ -16,6 +16,7 @@
 
 import {BaseElement} from '../src/base-element';
 import {Layout, isLayoutSizeDefined} from '../src/layout';
+import {ReadyState} from '../src/ready-state';
 import {Services} from '../src/services';
 import {dev} from '../src/log';
 import {guaranteeSrcForSrcsetUnsupportedBrowsers} from '../src/utils/img';
@@ -45,11 +46,39 @@ const ATTRIBUTES_TO_PROPAGATE = [
 ];
 
 export class AmpImg extends BaseElement {
+  /** @override @nocollapse */
+  static V1() {
+    return V1_IMG_DEFERRED_BUILD;
+  }
+
   /** @override @nocollapse */
   static prerenderAllowed() {
     return true;
   }
 
+  /** @override @nocollapse */
+  static getPreconnects(element) {
+    const src = element.getAttribute('src');
+    if (src) {
+      return [src];
+    }
+
+    // NOTE(@wassgha): since parseSrcset is computationally expensive and can
+    // not be inside the `buildCallback`, we went with preconnecting to the
+    // `src` url if it exists or the first srcset url.
+    const srcset = element.getAttribute('srcset');
+    if (srcset) {
+      // We try to find the first url in the srcset
+      const srcseturl = /\S+/.exec(srcset);
+      // Connect to the first url if it exists
+      if (srcseturl) {
+        return [srcseturl[0]];
+      }
+    }
+
+    return null;
+  }
+
   /** @param {!AmpElement} element */
   constructor(element) {
     super(element);
@@ -106,6 +135,10 @@ export class AmpImg extends BaseElement {
       if (!IS_ESM) {
         guaranteeSrcForSrcsetUnsupportedBrowsers(this.img_);
       }
+
+      if (AmpImg.V1() && !this.img_.complete) {
+        this.setReadyState(ReadyState.LOADING);
+      }
     }
   }
 
@@ -260,6 +293,38 @@ export class AmpImg extends BaseElement {
     return false;
   }
 
+  /** @override */
+  buildCallback() {
+    if (!AmpImg.V1()) {
+      return;
+    }
+
+    // A V1 amp-img loads and reloads automatically.
+    this.setReadyState(ReadyState.LOADING);
+    this.initialize_();
+    const img = dev().assertElement(this.img_);
+    if (img.complete) {
+      this.setReadyState(ReadyState.COMPLETE);
+      this.firstLayoutCompleted();
+      this.hideFallbackImg_();
+    }
+    listen(img, 'load', () => {
+      this.setReadyState(ReadyState.COMPLETE);
+      this.firstLayoutCompleted();
+      this.hideFallbackImg_();
+    });
+    listen(img, 'error', (reason) => {
+      this.setReadyState(ReadyState.ERROR, reason);
+      this.onImgLoadingError_();
+    });
+  }
+
+  /** @override */
+  ensureLoaded() {
+    const img = dev().assertElement(this.img_);
+    img.loading = 'eager';
+  }
+
   /** @override */
   layoutCallback() {
     this.initialize_();
@@ -275,6 +340,12 @@ export class AmpImg extends BaseElement {
 
   /** @override */
   unlayoutCallback() {
+    if (AmpImg.V1()) {
+      // TODO(#31915): Reconsider if this is still desired for V1. This helps
+      // with network interruption when a document is inactivated.
+      return;
+    }
+
     if (this.unlistenError_) {
       this.unlistenError_();
       this.unlistenError_ = null;
@@ -319,10 +390,8 @@ export class AmpImg extends BaseElement {
       !this.allowImgLoadFallback_ &&
       this.img_.classList.contains('i-amphtml-ghost')
     ) {
-      this.getVsync().mutate(() => {
-        this.img_.classList.remove('i-amphtml-ghost');
-        this.toggleFallback(false);
-      });
+      this.img_.classList.remove('i-amphtml-ghost');
+      this.toggleFallback(false);
     }
   }
 
@@ -332,13 +401,11 @@ export class AmpImg extends BaseElement {
    */
   onImgLoadingError_() {
     if (this.allowImgLoadFallback_) {
-      this.getVsync().mutate(() => {
-        this.img_.classList.add('i-amphtml-ghost');
-        this.toggleFallback(true);
-        // Hide placeholders, as browsers that don't support webp
-        // Would show the placeholder underneath a transparent fallback
-        this.togglePlaceholder(false);
-      });
+      this.img_.classList.add('i-amphtml-ghost');
+      this.toggleFallback(true);
+      // Hide placeholders, as browsers that don't support webp
+      // Would show the placeholder underneath a transparent fallback
+      this.togglePlaceholder(false);
       this.allowImgLoadFallback_ = false;
     }
   }

diff --git a/extensions/amp-auto-ads/0.1/placement.js b/extensions/amp-auto-ads/0.1/placement.js
@@ -213,7 +213,7 @@ export class Placement {
           return (
             whenUpgradedToCustomElement(this.getAdElement())
               // Responsive ads set their own size when built.
-              .then(() => this.getAdElement().whenBuilt())
+              .then(() => this.getAdElement().build())
               .then(() => {
                 const resized = !this.getAdElement().classList.contains(
                   'i-amphtml-layout-awaiting-size'
@@ -231,7 +231,7 @@ export class Placement {
           // synchronously. So we explicitly wait for CustomElement to be
           // ready.
           return whenUpgradedToCustomElement(this.getAdElement())
-            .then(() => this.getAdElement().whenBuilt())
+            .then(() => this.getAdElement().build())
             .then(() => {
               return this.mutator_.requestChangeSize(
                 this.getAdElement(),

diff --git a/extensions/amp-consent/0.1/consent-ui.js b/extensions/amp-consent/0.1/consent-ui.js
@@ -295,7 +295,7 @@ export class ConsentUI {
       // at build time. (see #18841).
       if (isAmpElement(this.ui_)) {
         whenUpgradedToCustomElement(this.ui_)
-          .then(() => this.ui_.whenBuilt())
+          .then(() => this.ui_.build())
           .then(() => show());
       } else {
         show();

diff --git a/extensions/amp-form/0.1/amp-form.js b/extensions/amp-form/0.1/amp-form.js
@@ -399,7 +399,7 @@ export class AmpForm {
       EXTERNAL_DEPS.join(',')
     );
     // Wait for an element to be built to make sure it is ready.
-    const promises = toArray(depElements).map((el) => el.whenBuilt());
+    const promises = toArray(depElements).map((el) => el.build());
     return (this.dependenciesPromise_ = this.waitOnPromisesOrTimeout_(
       promises,
       2000

diff --git a/extensions/amp-form/0.1/test/test-amp-form.js b/extensions/amp-form/0.1/test/test-amp-form.js
@@ -15,7 +15,6 @@
  */
 
 import '../../../amp-mustache/0.1/amp-mustache';
-import '../../../amp-selector/0.1/amp-selector';
 import * as xhrUtils from '../../../../src/utils/xhr-utils';
 import {ActionService} from '../../../../src/service/action-impl';
 import {ActionTrust} from '../../../../src/action-constants';
@@ -25,6 +24,7 @@ import {
   AmpFormService,
   checkUserValidityAfterInteraction_,
 } from '../amp-form';
+import {AmpSelector} from '../../../amp-selector/0.1/amp-selector';
 import {
   AsyncInputAttributes,
   AsyncInputClasses,
@@ -2287,74 +2287,70 @@ describes.repeated(
           });
         });
 
-        it('should submit after timeout of waiting for amp-selector', function () {
+        it('should submit after timeout of waiting for amp-selector', async function () {
           expectAsyncConsoleError(/Form submission failed/);
           this.timeout(3000);
-          return getAmpForm(getForm()).then((ampForm) => {
-            const form = ampForm.form_;
-            const selector = createElement('amp-selector');
-            selector.setAttribute('name', 'color');
-            form.appendChild(selector);
 
-            env.sandbox
-              .stub(selector, 'whenBuilt')
-              .returns(new Promise((unusedResolve) => {}));
-            env.sandbox.spy(ampForm, 'handleSubmitAction_');
+          const ampForm = await getAmpForm(getForm());
+          const form = ampForm.form_;
+          const selector = createElement('amp-selector');
+          selector.setAttribute('name', 'color');
 
-            const submitPromise = ampForm.actionHandler_({
-              method: 'submit',
-              satisfiesTrust: () => true,
-            });
-            expect(ampForm.handleSubmitAction_).to.have.not.been.called;
-            return timer
-              .promise(1)
-              .then(() => {
-                expect(ampForm.handleSubmitAction_).to.have.not.been.called;
-                return timer.promise(2000);
-              })
-              .then(() => {
-                expect(ampForm.handleSubmitAction_).to.have.been.calledOnce;
-                return submitPromise;
-              });
+          env.sandbox
+            .stub(AmpSelector.prototype, 'buildCallback')
+            .returns(new Promise((unusedResolve) => {}));
+          env.sandbox.spy(ampForm, 'handleSubmitAction_');
+
+          form.appendChild(selector);
+
+          const submitPromise = ampForm.actionHandler_({
+            method: 'submit',
+            satisfiesTrust: () => true,
           });
+          expect(ampForm.handleSubmitAction_).to.have.not.been.called;
+
+          await timer.promise(1);
+          expect(ampForm.handleSubmitAction_).to.have.not.been.called;
+
+          await timer.promise(2000);
+          expect(ampForm.handleSubmitAction_).to.have.been.calledOnce;
+
+          await submitPromise;
         });
 
-        it('should wait for amp-selector to build before submitting', () => {
-          return getAmpForm(getForm()).then((ampForm) => {
-            let builtPromiseResolver_;
-            const form = ampForm.form_;
-            const selector = createElement('amp-selector');
-            selector.setAttribute('name', 'color');
-            form.appendChild(selector);
+        it('should wait for amp-selector to build before submitting', async () => {
+          const ampForm = await getAmpForm(getForm());
 
-            env.sandbox.stub(selector, 'whenBuilt').returns(
-              new Promise((resolve) => {
-                builtPromiseResolver_ = resolve;
-              })
-            );
-            env.sandbox.stub(ampForm.xhr_, 'fetch').resolves();
-            env.sandbox.spy(ampForm, 'handleSubmitAction_');
+          let builtPromiseResolver_;
+          const form = ampForm.form_;
+          const selector = createElement('amp-selector');
+          selector.setAttribute('name', 'color');
 
-            ampForm.actionHandler_({
-              method: 'submit',
-              satisfiesTrust: () => true,
-            });
-            expect(ampForm.handleSubmitAction_).to.have.not.been.called;
-            return timer
-              .promise(1)
-              .then(() => {
-                expect(ampForm.handleSubmitAction_).to.have.not.been.called;
-                return timer.promise(100);
-              })
-              .then(() => {
-                expect(ampForm.handleSubmitAction_).to.have.not.been.called;
-                builtPromiseResolver_();
-                return timer.promise(1);
-              })
-              .then(() => {
-                expect(ampForm.handleSubmitAction_).to.have.been.calledOnce;
-              });
+          env.sandbox.stub(AmpSelector.prototype, 'buildCallback').returns(
+            new Promise((resolve) => {
+              builtPromiseResolver_ = resolve;
+            })
+          );
+          env.sandbox.stub(ampForm.xhr_, 'fetch').resolves();
+          env.sandbox.spy(ampForm, 'handleSubmitAction_');
+
+          form.appendChild(selector);
+
+          ampForm.actionHandler_({
+            method: 'submit',
+            satisfiesTrust: () => true,
           });
+          expect(ampForm.handleSubmitAction_).to.have.not.been.called;
+
+          await timer.promise(1);
+          expect(ampForm.handleSubmitAction_).to.have.not.been.called;
+
+          await timer.promise(100);
+          expect(ampForm.handleSubmitAction_).to.have.not.been.called;
+          builtPromiseResolver_();
+
+          await timer.promise(1);
+          expect(ampForm.handleSubmitAction_).to.have.been.calledOnce;
         });
 
         describe('Var Substitution', () => {

diff --git a/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js b/extensions/amp-sticky-ad/1.0/amp-sticky-ad.js
@@ -71,7 +71,7 @@ class AmpStickyAd extends AMP.BaseElement {
       dev().assertElement(this.ad_)
     )
       .then((ad) => {
-        return ad.whenBuilt();
+        return ad.build();
       })
       .then(() => {
         return this.mutateElement(() => {
@@ -188,7 +188,7 @@ class AmpStickyAd extends AMP.BaseElement {
    */
   scheduleLayoutForAd_() {
     whenUpgradedToCustomElement(dev().assertElement(this.ad_)).then((ad) => {
-      ad.whenBuilt().then(this.layoutAd_.bind(this));
+      ad.build().then(() => this.layoutAd_());
     });
   }