🚀 Delay marking unresolved AMP Elements until extension fails (#36157)

jridgewell · web-flow · commit 7959b22cc068 · 2021-09-24T21:22:20.000Z
diff --git a/extensions/amp-a4a/0.1/test/test-template-renderer.js b/extensions/amp-a4a/0.1/test/test-template-renderer.js
@@ -115,9 +115,8 @@ describes.realWin('TemplateRenderer', realWinConfig, (env) => {
               '"https://buy.com/buy-1">Click for ad!</a>' +
               '\n    <amp-analytics class="i-amphtml-element i-amphtml' +
               '-notbuilt amp-notbuilt i-amphtml-layout-fixed i-amphtml' +
-              '-layout-size-defined amp-unresolved i-amphtml-' +
-              'unresolved" i-amphtml-layout="fixed" style="width: 1px;' +
-              ' height: 1px;"></amp-analytics></div>'
+              '-layout-size-defined" i-amphtml-layout="fixed" style="width: ' +
+              '1px; height: 1px;"></amp-analytics></div>'
           );
         });
     });
diff --git a/src/amp.js b/src/amp.js
@@ -6,6 +6,7 @@
 import './polyfills';
 
 import {TickLabel} from '#core/constants/enums';
+import {whenDocumentComplete} from '#core/document-ready';
 import * as mode from '#core/mode';
 
 import {Services} from '#service';
@@ -21,6 +22,7 @@ import {installPlatformService} from '#service/platform-impl';
 
 import {installAutoLightboxExtension} from './auto-lightbox';
 import {startupChunk} from './chunk';
+import {markUnresolvedElements} from './custom-element';
 import {installErrorReporting} from './error-reporting';
 import {fontStylesheetTimeout} from './font-stylesheet-timeout';
 import {maybeTrackImpression} from './impression';
@@ -63,6 +65,7 @@ function bootstrap(ampdoc, perf) {
   startupChunk(self.document, function stub() {
     // Pre-stub already known elements.
     stubElementsForDoc(ampdoc);
+    whenDocumentComplete(self.document).then(() => markUnresolvedElements());
   });
   startupChunk(
     self.document,
diff --git a/src/custom-element.js b/src/custom-element.js
@@ -55,9 +55,17 @@ const RETURN_TRUE = () => true;
  */
 let templateTagSupported;
 
-/** @type {!Array} */
+/** @type {!Array<AmpElement>} */
 export const stubbedElements = [];
 
+/**
+ * Extensions which have failed to load, making their elements unresolvable.
+ * If null, then any remaining elements which don't immediately have their
+ * implClass available are marked unresolvable.
+ * @type {Set<string>}
+ */
+const unresolvableExtensions = new Set();
+
 /**
  * Whether this platform supports template tags.
  * @return {boolean}
@@ -367,6 +375,18 @@ function createBaseCustomElementClass(win, elementConnectedCallback) {
       }
     }
 
+    /**
+     * When the document is ready (meaning all external resources are loaded or
+     * failed), we mark any stubbed elements as unresolved. If they haven't
+     * been upgraded yet (or pending upgrade or deferredBuild elements), then
+     * the extension failed to load.
+     */
+    markUnresolved() {
+      if (!this.implClass_) {
+        this.classList.add('amp-unresolved', 'i-amphtml-unresolved');
+      }
+    }
+
     /**
      * Time delay imposed by baseElement upgradeCallback.  If no
      * upgradeCallback specified or not yet executed, delay is 0.
@@ -1182,11 +1202,17 @@ function createBaseCustomElementClass(win, elementConnectedCallback) {
         } catch (e) {
           reportError(e, this);
         }
+
         if (this.implClass_) {
           this.upgradeOrSchedule_();
+        } else if (
+          unresolvableExtensions.has('*') ||
+          unresolvableExtensions.has(this.tagName.toLowerCase())
+        ) {
+          this.markUnresolved();
         }
+
         if (!this.isUpgraded()) {
-          this.classList.add('amp-unresolved', 'i-amphtml-unresolved');
           this.dispatchCustomEventForTesting(AmpEvents.STUBBED);
         }
       }
@@ -2175,3 +2201,21 @@ export function getImplSyncForTesting(element) {
 export function getActionQueueForTesting(element) {
   return element.actionQueue_;
 }
+
+/**
+ * Marks each element that still stubbed as unresolved.
+ * @param {string=} opt_extension
+ */
+export function markUnresolvedElements(opt_extension) {
+  unresolvableExtensions.add(opt_extension || '*');
+  for (const el of stubbedElements) {
+    if (opt_extension == null || el.tagName.toLowerCase() === opt_extension) {
+      el.markUnresolved();
+    }
+  }
+}
+
+/** */
+export function resetUnresolvedElementsForTesting() {
+  unresolvableExtensions.clear();
+}
diff --git a/src/service/custom-element-registry.js b/src/service/custom-element-registry.js
@@ -2,7 +2,11 @@ import {Services} from '#service';
 
 import {extensionScriptsInNode} from './extension-script';
 
-import {createCustomElementClass, stubbedElements} from '../custom-element';
+import {
+  createCustomElementClass,
+  markUnresolvedElements,
+  stubbedElements,
+} from '../custom-element';
 import {ElementStub} from '../element-stub';
 import {reportError} from '../error-reporting';
 import {userAssert} from '../log';
@@ -120,8 +124,9 @@ function waitReadyForUpgrade(win, elementClass) {
  */
 export function stubElementsForDoc(ampdoc) {
   const extensions = extensionScriptsInNode(ampdoc.getHeadNode());
-  extensions.forEach(({extensionId, extensionVersion}) => {
+  extensions.forEach(({extensionId, extensionVersion, script}) => {
     ampdoc.declareExtension(extensionId, extensionVersion);
+    script.addEventListener('error', () => markUnresolvedElements(extensionId));
     stubElementIfNotKnown(ampdoc.win, extensionId);
   });
   if (ampdoc.isBodyAvailable()) {
diff --git a/src/service/extension-script.js b/src/service/extension-script.js
@@ -197,7 +197,7 @@ export function getExtensionScripts(
 /**
  * Get list of all the extension JS files.
  * @param {HTMLHeadElement|Element|ShadowRoot|Document} head
- * @return {!Array<{extensionId: string, extensionVersion: string}>}
+ * @return {!Array<{script: HTMLScriptElement, extensionId: string, extensionVersion: string}>}
  */
 export function extensionScriptsInNode(head) {
   // ampdoc.getHeadNode() can return null.
@@ -216,9 +216,12 @@ export function extensionScriptsInNode(head) {
       script.getAttribute('custom-element') ||
       script.getAttribute('custom-template');
     const urlParts = parseExtensionUrl(script.src);
-    // TODO: register error handler
     if (extensionId && urlParts) {
-      scripts.push({extensionId, extensionVersion: urlParts.extensionVersion});
+      scripts.push({
+        script,
+        extensionId,
+        extensionVersion: urlParts.extensionVersion,
+      });
     }
   }
   return scripts;
diff --git a/src/service/performance-impl.js b/src/service/performance-impl.js
@@ -144,7 +144,7 @@ export class Performance {
      * See https://github.com/GoogleChrome/web-vitals/blob/main/src/getCLS.ts
      * @private {Array<LayoutShift>}
      */
-    this.layoutShiftEntires_ = [];
+    this.layoutShiftEntries_ = [];
 
     /**
      * The sum of all layout shifts.
@@ -559,7 +559,7 @@ export class Performance {
     if (this.isVisibilityHidden_()) {
       return;
     }
-    const entries = this.layoutShiftEntires_;
+    const entries = this.layoutShiftEntries_;
     if (entries.length > 0) {
       const first = entries[0];
       const last = entries[entries.length - 1];
@@ -586,7 +586,7 @@ export class Performance {
    * See https://web.dev/evolving-cls/
    */
   flushLayoutShiftScore_() {
-    const entries = this.layoutShiftEntires_;
+    const entries = this.layoutShiftEntries_;
     const old = this.metrics_.get(TickLabel.CUMULATIVE_LAYOUT_SHIFT);
     let union = 0;
     let sum = 0;
diff --git a/test/unit/test-custom-element-registry.js b/test/unit/test-custom-element-registry.js
@@ -262,6 +262,7 @@ describes.realWin('CustomElement register', {amp: true}, (env) => {
         hasAttribute(name) {
           return name == 'custom-element' || name == 'src';
         },
+        addEventListener() {},
         src: 'https://cdn.ampproject.org/v0/amp-test1-0.2.js',
         ownerDocument: doc,
       };
@@ -326,6 +327,7 @@ describes.realWin('CustomElement register', {amp: true}, (env) => {
         hasAttribute(name) {
           return name == 'custom-element' || name == 'src';
         },
+        addEventListener() {},
         src: 'https://cdn.ampproject.org/v0/amp-test2-0.3.js',
         ownerDocument: doc,
       };
diff --git a/test/unit/test-custom-element.js b/test/unit/test-custom-element.js
@@ -13,6 +13,8 @@ import {chunkInstanceForTesting} from '../../src/chunk';
 import {
   createAmpElementForTesting,
   getImplSyncForTesting,
+  markUnresolvedElements,
+  resetUnresolvedElementsForTesting,
 } from '../../src/custom-element';
 import {ElementStub} from '../../src/element-stub';
 
@@ -153,6 +155,7 @@ describes.realWin('CustomElement', {amp: true}, (env) => {
       afterEach(() => {
         clock.uninstall();
         resourcesMock.verify();
+        resetUnresolvedElementsForTesting();
       });
 
       function skipMicroTask() {
@@ -948,9 +951,9 @@ describes.realWin('CustomElement', {amp: true}, (env) => {
 
         expect(element.everAttached).to.equal(true);
         expect(element.getLayout()).to.equal(Layout.FILL);
-        // Not upgraded yet!
-        expect(element).to.have.class('amp-unresolved');
-        expect(element).to.have.class('i-amphtml-unresolved');
+        // Not upgraded yet, but extension hasn't failed.
+        expect(element).not.to.have.class('amp-unresolved');
+        expect(element).not.to.have.class('i-amphtml-unresolved');
 
         // Upgrade
         resourcesMock.expects('upgraded').withExactArgs(element).once();
@@ -967,6 +970,44 @@ describes.realWin('CustomElement', {amp: true}, (env) => {
         expect(element).to.not.have.class('i-amphtml-unresolved');
       });
 
+      it('StubElement - attachedCallback after failed to load', () => {
+        const element = new StubElementClass();
+        markUnresolvedElements('amp-stub');
+        element.setAttribute('layout', 'fill');
+        expect(element.everAttached).to.equal(false);
+        expect(element.getLayout()).to.equal(Layout.NODISPLAY);
+
+        resourcesMock.expects('add').withExactArgs(element).atLeast(1);
+        container.appendChild(element);
+
+        expect(element.everAttached).to.equal(true);
+        expect(element.getLayout()).to.equal(Layout.FILL);
+        // Extension already failed before attachedCallback
+        expect(element).to.have.class('amp-unresolved');
+        expect(element).to.have.class('i-amphtml-unresolved');
+      });
+
+      it('StubElement - attachedCallback before failed to load', () => {
+        const element = new StubElementClass();
+        element.setAttribute('layout', 'fill');
+        expect(element.everAttached).to.equal(false);
+        expect(element.getLayout()).to.equal(Layout.NODISPLAY);
+
+        resourcesMock.expects('add').withExactArgs(element).atLeast(1);
+        container.appendChild(element);
+
+        expect(element.everAttached).to.equal(true);
+        expect(element.getLayout()).to.equal(Layout.FILL);
+        // Not upgraded yet, but extension hasn't failed.
+        expect(element).not.to.have.class('amp-unresolved');
+        expect(element).not.to.have.class('i-amphtml-unresolved');
+
+        // Now it's called.
+        markUnresolvedElements('amp-stub');
+        expect(element).to.have.class('amp-unresolved');
+        expect(element).to.have.class('i-amphtml-unresolved');
+      });
+
       it('Element - detachedCallback', () => {
         const element = new ElementClass();
         element.setAttribute('layout', 'fill');
