amp-state: Remove restriction that amp-state elements must have a JSON descendant or a src attriute, but not both

extensions/amp-bind/0.1/amp-state.js

@@ -16,13 +16,26 @@
 
 import {bindForDoc, viewerForDoc} from '../../../src/services';
 import {fetchBatchedJsonFor} from '../../../src/batched-json';
+import {getMode} from '../../../src/mode';
 import {isBindEnabledFor} from './bind-impl';
 import {isJsonScriptTag} from '../../../src/dom';
 import {toggle} from '../../../src/style';
 import {tryParseJson} from '../../../src/json';
 import {dev, user} from '../../../src/log';
 
 export class AmpState extends AMP.BaseElement {
+
+  /** @param {!AmpElement} element */
+  constructor(element) {
+    super(element);
+
+    if (getMode().test) {
+      /** @visibleForTesting {?Promise} */
+      this.updateStatePromise = null;
+    }
+  }
+
+
   /** @override */
   getPriority() {
     // Loads after other content.
@@ -71,7 +84,10 @@ export class AmpState extends AMP.BaseElement {
     }
     const src = mutations['src'];
     if (src !== undefined) {
-      this.fetchSrcAndUpdateState_(/* isInit */ false);
+      const p = this.fetchSrcAndUpdateState_(/* isInit */ false);
+      if (getMode().test) {
+        this.updateStatePromise = p;
+      }
     }
   }
 
@@ -87,36 +103,63 @@ export class AmpState extends AMP.BaseElement {
 
     // Fetch JSON from endpoint at `src` attribute if it exists,
     // otherwise parse child script tag.
+    // If both `src` and child script tag are provided,
+    // state fetched from `src` takes precedence.
+    const children = this.element.children;
+    if (children.length == 0 && !this.element.hasAttribute('src')) {
+      user().error(TAG, 'Needs either a <script> child or src attribute');
+    }
+    if (children.length == 1) {
+      this.parseChildAndUpdateState_();
+    } else if (children.length > 1) {
+      user().error(TAG, 'Should contain only one <script> child.');
+    }
     if (this.element.hasAttribute('src')) {
-      this.fetchSrcAndUpdateState_(/* isInit */ true);
-      if (this.element.children.length > 0) {
-        user().error(TAG, 'Should not have children if src attribute exists.');
+      const p = this.fetchSrcAndUpdateState_(/* isInit */ true);
+      if (getMode().test) {
+        this.updateStatePromise = p;
       }
+    }
+
+  }
+
+  /**
+   * @private
+   */
+  parseChildAndUpdateState_() {
+    const TAG = this.getName_();
+    const children = this.element.children;
+    const firstChild = children[0];
+    if (isJsonScriptTag(firstChild)) {
+      const json = tryParseJson(firstChild.textContent, e => {
+        user().error(TAG, 'Failed to parse state. Is it valid JSON?', e);
+      });
+      this.updateState_(json, /* isInit */ true);
     } else {
-      const children = this.element.children;
-      if (children.length == 1) {
-        const firstChild = children[0];
-        if (isJsonScriptTag(firstChild)) {
-          const json = tryParseJson(firstChild.textContent, e => {
-            user().error(TAG, 'Failed to parse state. Is it valid JSON?', e);
-          });
-          this.updateState_(json, /* isInit */ true);
-        } else {
-          user().error(TAG,
-              'State should be in a <script> tag with type="application/json"');
-        }
-      } else if (children.length > 1) {
-        user().error(TAG, 'Should contain only one <script> child.');
-      }
+      user().error(TAG,
+          'State should be in a <script> tag with type="application/json"');
     }
   }
 
+  /**
+   * Wrapper to stub during testing.
+   * @param {!../../../src/service/ampdoc-impl.AmpDoc} ampdoc
+   * @param {!Element} element
+   * @return {!Promise}
+   * @visibleForTesting
+   */
+  fetchBatchedJsonFor_(ampdoc, element) {
+    return fetchBatchedJsonFor(ampdoc, element);
+  }
+
   /**
    * @param {boolean} isInit
+   * @returm {!Promise}
    * @private
    */
   fetchSrcAndUpdateState_(isInit) {
-    fetchBatchedJsonFor(this.getAmpDoc(), this.element).then(json => {
+    const ampdoc = this.getAmpDoc();
+    return this.fetchBatchedJsonFor_(ampdoc, this.element).then(json => {
       this.updateState_(json, isInit);
     });
   }

extensions/amp-bind/0.1/test/test-amp-state.js

@@ -24,7 +24,8 @@ describes.realWin('AmpState', {
   },
 }, env => {
   let ampState;
-  let fetchStub;
+  let fetchSpy;
+  let batchedJsonStub;
   let updateStub;
 
   // Viewer-related vars.
@@ -45,50 +46,78 @@ describes.realWin('AmpState', {
     whenFirstVisiblePromise = new Promise(resolve => {
       whenFirstVisiblePromiseResolve = resolve;
     });
+
     env.sandbox.stub(viewer, 'whenFirstVisible', () => whenFirstVisiblePromise);
 
     ampState = getAmpState();
-
     const impl = ampState.implementation_;
 
-    // For simpler testing, stub the fetching and update call to Bind service.
-    // - `fetchStub should only be called when fetching remote JSON data
-    // - `updateStub` should only be called when parsing a child script
-    fetchStub = sandbox.stub(impl, 'fetchSrcAndUpdateState_');
-    updateStub = sandbox.stub(impl, 'updateState_');
+    fetchSpy = env.sandbox.spy(impl, 'fetchSrcAndUpdateState_');
+    batchedJsonStub = env.sandbox.stub(impl, 'fetchBatchedJsonFor_')
+        .returns(Promise.resolve({baz: 'qux'}));
+    updateStub = env.sandbox.stub(impl, 'updateState_');
   });
 
-  it('should fetch json if `src` attribute exists', () => {
+  afterEach(() => {
+    env.sandbox.restore();
+  });
+
+  it('should fetch json at build if `src` attribute exists', () => {
     ampState.setAttribute('src', 'https://foo.com/bar?baz=1');
     ampState.build();
 
     // IMPORTANT: No CORS fetch should happen until viewer is visible.
-    expect(fetchStub).to.not.have.been.called;
+    expect(fetchSpy).to.not.have.been.called;
+    expect(batchedJsonStub).to.not.have.been.called;
     expect(updateStub).to.not.have.been.called;
 
     whenFirstVisiblePromiseResolve();
     return whenFirstVisiblePromise.then(() => {
-      expect(fetchStub).calledWithExactly(/* isInit */ true);
-      expect(updateStub).to.not.have.been.called;
+      expect(fetchSpy).calledWithExactly(/* isInit */ true);
+      return ampState.implementation_.updateStatePromise;
+    }).then(() => {
+      expect(updateStub).calledWithMatch({baz: 'qux'});
     });
   });
 
-  it('should parse its child script if `src` attribute does not exist', () => {
+  it('should parse its child script', () => {
     ampState.innerHTML = '<script type="application/json">' +
         '{"foo": "bar"}</script>';
     ampState.build();
 
     // IMPORTANT: No parsing should happen until viewer is visible.
-    expect(fetchStub).to.not.have.been.called;
+    expect(fetchSpy).to.not.have.been.called;
+    expect(batchedJsonStub).to.not.have.been.called;
     expect(updateStub).to.not.have.been.called;
 
     whenFirstVisiblePromiseResolve();
     return whenFirstVisiblePromise.then(() => {
-      expect(fetchStub).to.not.have.been.called;
+      expect(fetchSpy).to.not.have.been.called;
       expect(updateStub).calledWithMatch({foo: 'bar'});
     });
   });
 
+  it('should parse child and fetch `src` if both provided at build', () => {
+    ampState.innerHTML = '<script type="application/json">' +
+        '{"foo": "bar"}</script>';
+    ampState.setAttribute('src', 'https://foo.com/bar?baz=1');
+    ampState.build();
+
+    // IMPORTANT: No parsing should happen until viewer is visible.
+    expect(fetchSpy).to.not.have.been.called;
+    expect(batchedJsonStub).to.not.have.been.called;
+    expect(updateStub).to.not.have.been.called;
+
+    whenFirstVisiblePromiseResolve();
+    return whenFirstVisiblePromise.then(() => {
+      expect(updateStub).calledWithMatch({foo: 'bar'});
+      expect(fetchSpy).calledWithExactly(/* isInit */ true);
+      return ampState.implementation_.updateStatePromise;
+    }).then(() => {
+      expect(updateStub).calledWithMatch({baz: 'qux'});
+    });
+  });
+
   it('should fetch json if `src` is mutated', () => {
     ampState.setAttribute('src', 'https://foo.com/bar?baz=1');
     ampState.build();
@@ -97,10 +126,14 @@ describes.realWin('AmpState', {
     const isVisibleStub = env.sandbox.stub(viewer, 'isVisible');
     isVisibleStub.returns(false);
     ampState.mutatedAttributesCallback({src: 'https://foo.com/bar?baz=1'});
-    expect(fetchStub).to.not.have.been.called;
+    expect(fetchSpy).to.not.have.been.called;
+    expect(batchedJsonStub).to.not.have.been.called;
 
     isVisibleStub.returns(true);
     ampState.mutatedAttributesCallback({src: 'https://foo.com/bar?baz=1'});
-    expect(fetchStub).calledWithExactly(/* isInit */ false);
+    expect(fetchSpy).calledWithExactly(/* isInit */ false);
+    return ampState.implementation_.updateStatePromise.then(() => {
+      expect(updateStub).calledWithMatch({baz: 'qux'});
+    });
   });
 });

extensions/amp-bind/0.1/test/validator-amp-bind.html

@@ -58,5 +58,17 @@ <h2>Basic example</h2>
       }
     </script>
   </amp-state>
+
+  <amp-state id="myState2" src="https://www.google.com/" credentials="omit">
+  </amp-state>
+
+  <amp-state id="myState3" src="https://www.google.com/" credentials="include">
+    <script type="application/json">
+      {
+        "myStateKey1": "myStateValue1"
+      }
+    </script>
+  </amp-state>
+
 </body>
 </html>

extensions/amp-bind/0.1/validator-amp-bind.protoascii

@@ -53,10 +53,19 @@ tags: {  # <amp-state> with json child
   requires: "amp-bind extension .js script"
   requires: "amp-bind extension .json script"
   disallowed_ancestor: "AMP-SIDEBAR"
+  attrs: { name: "credentials" }
   attrs: {
     name: "id"
     mandatory: true
   }
+  attrs: {
+    name: "src"
+    value_url: {
+      allowed_protocol: "https"
+      allow_relative: true  # Will be set to false at a future date.
+    }
+    blacklisted_value_regex: "__amp_source_origin"
+  }
   # <amp-bind>
   attrs: { name: "[src]" }
   child_tags: {
@@ -65,7 +74,7 @@ tags: {  # <amp-state> with json child
   }
   spec_url: "https://www.ampproject.org/docs/reference/components/amp-bind"
 }
-tags: {  # <amp-state> with src
+tags: {  # <amp-state> with src only
   html_format: AMP
   tag_name: "AMP-STATE"
   spec_name: "amp-state[src]"