🚀♻️ amp-story-shopping-config: no custom element (#37509)

Custom elements are heavy. `<amp-story-shopping-config>` does not render, or hold any state, so it does not need to be a component. Instead of registering a custom element, use a parser function instead. (Note that this retains API and all other behavior).
ampproject · Feb 2, 2022 · 9486cdb · 9486cdb
1 parent fb3fb36
commit 9486cdb
Show file tree

Hide file tree

Showing 4 changed files with 147 additions and 110 deletions.
diff --git a/extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js b/extensions/amp-story-shopping/0.1/amp-story-shopping-attachment.js
@@ -5,6 +5,10 @@ import {Services} from '#service';
 import {LocalizedStringId_Enum} from '#service/localization/strings';
 
 import {formatI18nNumber, loadFonts} from './amp-story-shopping';
+import {
+  getShoppingConfig,
+  storeShoppingConfig,
+} from './amp-story-shopping-config';
 
 import {
   ShoppingConfigDataDef,
@@ -55,6 +59,19 @@ export class AmpStoryShoppingAttachment extends AMP.BaseElement {
     this.shoppingTags_ = Array.from(
       this.pageEl_.querySelectorAll('amp-story-shopping-tag')
     );
+    loadFonts(this.win, FONTS_TO_LOAD);
+
+    const pageElement = this.element.parentElement;
+    getShoppingConfig(pageElement).then((config) =>
+      storeShoppingConfig(pageElement, config)
+    );
+
+    this.attachmentEl_ = (
+      <amp-story-page-attachment
+        layout="nodisplay"
+        theme={this.element.getAttribute('theme')}
+      ></amp-story-page-attachment>
+    );
     if (this.shoppingTags_.length === 0) {
       return;
     }

diff --git a/extensions/amp-story-shopping/0.1/amp-story-shopping-config.js b/extensions/amp-story-shopping/0.1/amp-story-shopping-config.js
@@ -1,5 +1,3 @@
-import {Layout_Enum} from '#core/dom/layout';
-
 import {Services} from '#service';
 
 import {getElementConfig} from 'extensions/amp-story/1.0/request-utils';
@@ -14,44 +12,46 @@ import {
  *  items: !Array<!ShoppingConfigDataDef>,
  * }}
  */
-let ShoppingConfigDef;
+let ShoppingConfigResponseDef;
 
-export class AmpStoryShoppingConfig extends AMP.BaseElement {
-  /** @param {!AmpElement} element */
-  constructor(element) {
-    super(element);
-    /** @private @const {?../../amp-story/1.0/amp-story-store-service.AmpStoryStoreService} */
-    this.storeService_ = null;
-  }
+/** @typedef {!Object<string, !ShoppingConfigDataDef> */
+export let KeyedShoppingConfigDef;
 
-  /**
-   * Keys product data to productIds and adds them to the store service.
-   * @param {!ShoppingConfigDef} shoppingConfig
-   * @private
-   */
-  addShoppingDataFromConfig_(shoppingConfig) {
-    const productIDtoProduct = {};
-    for (const item of shoppingConfig['items']) {
-      productIDtoProduct[item['productId']] = item;
-    }
-    this.storeService_.dispatch(Action.ADD_SHOPPING_DATA, productIDtoProduct);
+/**
+ * Gets Shopping config from an <amp-story-page> element.
+ * The config is validated and keyed by 'product-tag-id'.
+ * @param {!Element} pageElement <amp-story-page>
+ * @return {!Promise<!KeyedShoppingConfigDef>}
+ */
+export function getShoppingConfig(pageElement) {
+  const element = pageElement.querySelector('amp-story-shopping-config');
+  return getElementConfig(element).then((config) => {
     //TODO(#36412): Add call to validate config here.
-  }
+    return keyByProductTagId(config);
+  });
+}
 
-  /** @override */
-  buildCallback() {
-    super.buildCallback();
-    return Promise.all([
-      Services.storyStoreServiceForOrNull(this.win),
-      getElementConfig(this.element),
-    ]).then(([storeService, storyConfig]) => {
-      this.storeService_ = storeService;
-      this.addShoppingDataFromConfig_(storyConfig);
-    });
+/**
+ * @param {!ShoppingConfigResponseDef} config
+ * @return {!KeyedShoppingConfigDef}
+ */
+function keyByProductTagId(config) {
+  const keyed = {};
+  for (const item of config.items) {
+    keyed[item.productId] = item;
   }
+  return keyed;
+}
 
-  /** @override */
-  isLayoutSupported(layout) {
-    return layout === Layout_Enum.NODISPLAY;
-  }
+/**
+ * @param {!Element} pageElement
+ * @param {!KeyedShoppingConfigDef} config
+ * @return {!Promise<!ShoppingConfigResponseDef>}
+ */
+export function storeShoppingConfig(pageElement, config) {
+  const win = pageElement.ownerDocument.defaultView;
+  return Services.storyStoreServiceForOrNull(win).then((storeService) => {
+    storeService?.dispatch(Action.ADD_SHOPPING_DATA, config);
+    return config;
+  });
 }
diff --git a/extensions/amp-story-shopping/0.1/amp-story-shopping.js b/extensions/amp-story-shopping/0.1/amp-story-shopping.js
@@ -1,5 +1,4 @@
 import {AmpStoryShoppingAttachment} from './amp-story-shopping-attachment';
-import {AmpStoryShoppingConfig} from './amp-story-shopping-config';
 import {AmpStoryShoppingTag} from './amp-story-shopping-tag';
 
 import {CSS as shoppingCSS} from '../../../build/amp-story-shopping-0.1.css';
@@ -40,7 +39,6 @@ export const loadFonts = (win, fontFaces) => {
 };
 
 AMP.extension('amp-story-shopping', '0.1', (AMP) => {
-  AMP.registerElement('amp-story-shopping-config', AmpStoryShoppingConfig);
   AMP.registerElement(
     'amp-story-shopping-tag',
     AmpStoryShoppingTag,

diff --git a/extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-config.js b/extensions/amp-story-shopping/0.1/test/test-amp-story-shopping-config.js
@@ -1,13 +1,13 @@
-import {createElementWithAttributes} from '#core/dom';
-import {Layout_Enum} from '#core/dom/layout';
-import '../amp-story-shopping';
+import * as Preact from '#core/dom/jsx';
 
-import * as configData from '../../../../examples/amp-story/shopping/remote.json';
-import {registerServiceBuilder} from '../../../../src/service-helpers';
+import {Services} from '#service';
+
+import * as remoteConfig from '../../../../examples/amp-story/shopping/remote.json';
+import {Action} from '../../../amp-story/1.0/amp-story-store-service';
 import {
-  StateProperty,
-  getStoreService,
-} from '../../../amp-story/1.0/amp-story-store-service';
+  getShoppingConfig,
+  storeShoppingConfig,
+} from '../amp-story-shopping-config';
 
 describes.realWin(
   'amp-story-shopping-config-v0.1',
@@ -18,90 +18,112 @@ describes.realWin(
     },
   },
   (env) => {
-    let win;
-    let element;
-    let shoppingConfig;
-    let storeService;
+    let pageElement;
+
+    const defaultInlineConfig = {
+      items: [
+        {
+          'productId': 'city-pop',
+          'productTitle': 'Plastic Love',
+          'productPrice': 19,
+          'productPriceCurrency': 'JPY',
+        },
+      ],
+    };
+
+    const keyedDefaultInlineConfig = {
+      'city-pop': defaultInlineConfig.items[0],
+    };
 
     beforeEach(async () => {
-      win = env.win;
-      storeService = getStoreService(win);
-      registerServiceBuilder(win, 'story-store', function () {
-        return storeService;
-      });
-
-      await createAmpStoryShoppingConfig();
+      pageElement = <amp-story-page id="page1"></amp-story-page>;
+      env.win.document.body.appendChild(pageElement);
     });
 
-    async function createAmpStoryShoppingConfig() {
-      const pageEl = win.document.createElement('amp-story-page');
-      pageEl.id = 'page1';
-      element = createElementWithAttributes(
-        win.document,
-        'amp-story-shopping-config',
-        {'layout': 'nodisplay'}
+    async function createAmpStoryShoppingConfig(
+      src = null,
+      config = defaultInlineConfig
+    ) {
+      pageElement.appendChild(
+        <amp-story-shopping-config layout="nodisplay" src={src}>
+          <script type="application/json">{JSON.stringify(config)}</script>
+        </amp-story-shopping-config>
       );
-
-      element.innerHTML = `
-        <script type="application/json">
-          ${JSON.stringify(configData)}
-        </script>
-      `;
-
-      pageEl.appendChild(element);
-      win.document.body.appendChild(pageEl);
-
-      shoppingConfig = await element.getImpl();
+      return getShoppingConfig(pageElement);
     }
 
-    it('should build shopping config component', () => {
-      expect(() => shoppingConfig.layoutCallback()).to.not.throw();
-    });
-
     it('throws on no config', async () => {
       expectAsyncConsoleError(async () => {
-        expect(async () => {
-          await shoppingConfig.buildCallback();
-        }).to.throw(
-          /The amp-story-auto-ads:config should be inside a <script> tag with type=\"application\/json\"/
-        );
+        expect(() => {
+          pageElement.appendChild(<amp-story-shopping-config />);
+          return getShoppingConfig(pageElement);
+        }).to.throw(/<script> tag with type=\"application\/json\"/);
       });
     });
 
-    it('does use remote config when src attribute is provided', async () => {
-      const exampleURL = 'foo.example';
-      element.setAttribute('src', exampleURL);
-
-      const expectedRemoteResult = JSON.parse(
-        '{"art":{"productId": "art","productTitle": "Abstract Art","productBrand": "V. Artsy","productPrice": 1200.0,"productPriceCurrency": "JPY","productImages": ["https://source.unsplash.com/BdVQU-NDtA8/500x500"]}}'
-      );
+    it('does use inline config', async () => {
+      const result = await createAmpStoryShoppingConfig();
+      expect(result).to.deep.eql(keyedDefaultInlineConfig);
+    });
 
-      expect(storeService.get(StateProperty.SHOPPING_DATA)).to.deep.eql(
-        expectedRemoteResult
-      );
+    it('does use remote config when src attribute is provided', async () => {
+      const remoteUrl = 'https://foo.example';
+      const expectedRemoteResult =
+        // matches remote.json
+        {
+          'art': {
+            'productId': 'art',
+            'productTitle': 'Abstract Art',
+            'productBrand': 'V. Artsy',
+            'productPrice': 1200.0,
+            'productPriceCurrency': 'JPY',
+            'productImages': [
+              'https://source.unsplash.com/BdVQU-NDtA8/500x500',
+            ],
+          },
+        };
+      env.sandbox.stub(Services, 'xhrFor').returns({
+        fetchJson(url) {
+          if (url === remoteUrl) {
+            return Promise.resolve({
+              ok: true,
+              json: () => remoteConfig,
+            });
+          }
+        },
+      });
+      const result = await createAmpStoryShoppingConfig(remoteUrl);
+      expect(result).to.deep.eql(expectedRemoteResult);
     });
 
     it('does use inline config when remote src is invalid', async () => {
-      const exampleURL = 'invalidRemoteURL';
-      element.setAttribute('src', exampleURL);
-
-      shoppingConfig.buildCallback().then(async () => {
-        expect(await shoppingConfig.getInlineConfig_).to.be.called();
+      env.sandbox.stub(Services, 'xhrFor').returns({
+        fetchJson() {
+          throw new Error();
+        },
       });
+      const result = await createAmpStoryShoppingConfig('invalidRemoteUrl');
+      expect(result).to.deep.eql(keyedDefaultInlineConfig);
     });
 
-    it('Test Invalid Remote Config URL', async () => {
-      const exampleURL = 'invalidRemoteURL';
-      element.setAttribute('src', exampleURL);
-      expectAsyncConsoleError(async () => {
-        expect(async () => {
-          await shoppingConfig.buildCallback();
-        }).to.throw(
-          /'amp-story-auto-ads:config error determining if remote config is valid json: bad url or bad json'/
-        );
+    describe('storeShoppingConfig', () => {
+      let storeService;
+
+      beforeEach(async () => {
+        storeService = {dispatch: env.sandbox.spy()};
+        env.sandbox
+          .stub(Services, 'storyStoreServiceForOrNull')
+          .resolves(storeService);
+      });
+
+      it('dispatches ADD_SHOPPING_DATA', async () => {
+        const config = {foo: {bar: true}};
+
+        await storeShoppingConfig(pageElement, config);
+
+        expect(storeService.dispatch.withArgs(Action.ADD_SHOPPING_DATA, config))
+          .to.have.been.calledOnce;
       });
-      expect(shoppingConfig.isLayoutSupported(Layout_Enum.NODISPLAY)).to.be
-        .true;
     });
   }
 );