address choumx feedback

ampproject · Mar 12, 2020 · 5e92cc7 · 5e92cc7
1 parent 34e24dd
commit 5e92cc7
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 7 deletions.
diff --git a/build-system/global-configs/canary-config.json b/build-system/global-configs/canary-config.json
@@ -15,6 +15,7 @@
   "amp-auto-ads-adsense-holdout": 0.1,
   "amp-auto-ads-no-op-experiment": 0.05,
   "amp-consent-restrict-fullscreen": 1,
+  "amp-list-init-from-state": 1,
   "amp-mega-menu": 1,
   "amp-nested-menu": 1,
   "amp-playbuzz": 1,

diff --git a/build-system/global-configs/prod-config.json b/build-system/global-configs/prod-config.json
@@ -15,6 +15,7 @@
   "amp-auto-ads-adsense-holdout": 0.1,
   "amp-auto-ads-no-op-experiment": 0.05,
   "amp-consent-restrict-fullscreen": 1,
+  "amp-list-init-from-state": 1,
   "amp-mega-menu": 1,
   "amp-nested-menu": 1,
   "amp-playbuzz": 1,

diff --git a/examples/amp-list.state.html b/examples/amp-list.state.html
@@ -20,7 +20,6 @@
   </style>
 </head>
 <body>
-
   <div on="tap:list1.refresh,list2.refresh,list3.refresh">
     Refresh All Lists
   </div>

diff --git a/extensions/amp-list/0.1/amp-list.js b/extensions/amp-list/0.1/amp-list.js
@@ -60,6 +60,7 @@ import {
   setupJsonFetchInit,
 } from '../../../src/utils/xhr-utils';
 import {isArray, toArray} from '../../../src/types';
+import {isExperimentOn} from '../../../src/experiments';
 import {px, setStyles, toggle} from '../../../src/style';
 import {setDOM} from '../../../third_party/set-dom/set-dom';
 import {startsWith} from '../../../src/string';
@@ -351,7 +352,10 @@ export class AmpList extends AMP.BaseElement {
    * @private
    */
   isAmpStateSrc_(src) {
-    return startsWith(src, AMP_STATE_URI_SCHEME);
+    return (
+      isExperimentOn(this.win, 'amp-list-init-from-state') &&
+      startsWith(src, AMP_STATE_URI_SCHEME)
+    );
   }
 
   /**

diff --git a/extensions/amp-list/0.1/test/test-amp-list.js b/extensions/amp-list/0.1/test/test-amp-list.js
@@ -20,6 +20,10 @@ import {AmpEvents} from '../../../../src/amp-events';
 import {AmpList} from '../amp-list';
 import {Deferred} from '../../../../src/utils/promise';
 import {Services} from '../../../../src/services';
+import {
+  resetExperimentTogglesForTesting,
+  toggleExperiment,
+} from '../../../../src/experiments';
 
 describes.repeated(
   'amp-list',
@@ -806,6 +810,8 @@ describes.repeated(
             });
 
             it('"amp-state:" uri should skip rendering and emit an error', () => {
+              toggleExperiment(win, 'amp-list-init-from-state', true);
+
               const ampStateEl = doc.createElement('amp-state');
               ampStateEl.setAttribute('id', 'okapis');
               const ampStateJson = doc.createElement('script');
@@ -1224,14 +1230,24 @@ describes.repeated(
           });
 
           describe('Using amp-state: protocol', () => {
+            const experimentName = 'amp-list-init-from-state';
+
             beforeEach(() => {
+              resetExperimentTogglesForTesting(win);
               element = createAmpListElement();
               element.setAttribute('src', 'amp-state:okapis');
               element.toggleLoading = () => {};
               list = createAmpList(element);
             });
 
+            it('should throw an error if used without the experiment enabled', async () => {
+              const errorMsg = /Invalid value: amp-state:okapis/;
+              expectAsyncConsoleError(errorMsg);
+              expect(list.layoutCallback()).to.eventually.throw(errorMsg);
+            });
+
             it('should throw error if there is no associated amp-state el', async () => {
+              toggleExperiment(win, experimentName, true);
               bind.getStateAsync = () => Promise.reject();
 
               const errorMsg = /element with id 'okapis' was not found/;
@@ -1240,6 +1256,7 @@ describes.repeated(
             });
 
             it('should log an error if amp-bind was not included', async () => {
+              toggleExperiment(win, experimentName, true);
               Services.bindForDocOrNull.returns(Promise.resolve(null));
 
               const ampStateEl = doc.createElement('amp-state');
@@ -1255,6 +1272,7 @@ describes.repeated(
             });
 
             it('should render a list using local data', async () => {
+              toggleExperiment(win, experimentName, true);
               bind.getStateAsync = () => Promise.resolve({items: [1, 2, 3]});
 
               const ampStateEl = doc.createElement('amp-state');
@@ -1274,6 +1292,7 @@ describes.repeated(
             });
 
             it('should render a list using async data', async () => {
+              toggleExperiment(win, experimentName, true);
               const {resolve, promise} = new Deferred();
               bind.getStateAsync = () => promise;
 

diff --git a/extensions/amp-list/amp-list.md b/extensions/amp-list/amp-list.md
@@ -222,19 +222,32 @@ In several cases, we may need the `<amp-list>` to resize on user interaction. Fo
 
 ### Initialization from amp-state
 
-In some cases, it may be desirable to have your `<amp-list>` component initialize off of `<amp-state>` rather than from a json endpoint.
-You may do that by utilizing the `amp-state:` protocol in the `src` attribute. The data in state must follow the same rules as the json that
-would have been retrieved from an endpoint. For example,
+For cases where you can server-side-render the JSON for an `<amp-list>`, it may be
+desirable to have your `<amp-list>` component initialize off of `<amp-state>` rather
+than from a JSON endpoint. This allows the component to render faster by skipping the fetch,
+but also means the data may be stale if served from the AMP Cache.
+
+You may enable this by following two steps:
+
+1. Add the [amp-bind](https://amp.dev/documentation/components/amp-bind/) script to the `<head>` of your document.
+2. Utilize an `amp-state:` protocol in the `src` attribute. The data in state
+   must follow the same rules as the JSON that would have been retrieved from an endpoint.
+
+For example,
 
 ```html
-<amp-state id="listExample">
+<amp-state id="localState">
   <script type="application/json">
     {
       "items": [{"id": 1}, {"id": 2}, {"id": 2}]
     }
   </script>
 </amp-state>
-<amp-list src="amp-state:listExample">...</amp-list>
+<amp-list src="amp-state:localState">
+  <template type="amp-mustache">
+    <li>{{id}}</li>
+  </template>
+</amp-list>
 ```
 
 ## Attributes
-Original file line number
+Diff line change
@@ Expand Up / @@ -20,7 +20,6 @@ @@
       </style>
     </head>
     <body>
       <div on="tap:list1.refresh,list2.refresh,list3.refresh">
         Refresh All Lists
       </div>
@@ Expand Down @@