✨PWA: Expose amp-bind getState and setState by mattwomple · Pull Request #25011 · ampproject/amphtml

mattwomple · 2019-10-10T20:47:37Z

Allow PWAs to access amp-bind methods through the shadowDoc instance created by AMP.attachShadowDoc():

shadowDoc.getState(expr) - Get an amp-bind state from the AMP document using a JSON expression string
shadowDoc.setState(state) - Deep merge an object or expression into the AMP document's global amp-bind state

Expose shadowDoc.getState and shadowDoc.setState methods for PWAs.

mattwomple · 2019-10-10T20:48:46Z

@choumx Initial thoughts welcome. I'm actually blocked on testing .setState because this promise seems to hang (at least in localhost testing):

amphtml/extensions/amp-bind/0.1/bind-impl.js

Line 1024 in bcdd618

const evaluatePromise = this.ww_('bind.evaluateBindings', [this.state_]);

Oh, I see, ww.js isn't lazy building: GET http://localhost:8000/dist/ww.js net::ERR_EMPTY_RESPONSE

mattwomple · 2019-10-11T09:11:32Z

src/runtime.js

+       * a copy of the value of a state
+       */
+      getState: name => {
+        return Services.bindForDocOrNull(shadowRoot).then(bind => {


For pages that do not use amp-bind, this promise never resolves. I don't think it's a problem, but wanted to make sure this received consideration.

Hm, it should resolve with null on pages that are missing the extension script in the doc head.

mattwomple · 2019-10-11T09:11:44Z

src/runtime.js

+       * @return {Promise} - Resolves after state and history have been updated
+       */
+      setState: state => {
+        return Services.bindForDocOrNull(shadowRoot).then(bind => {


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

mattwomple · 2019-10-14T18:52:43Z

src/runtime.js

+       */
+      getState: name => {
+        return Services.bindForDocOrNull(shadowRoot).then(bind => {
+          return bind && bind.getStateCopy(name);


This should probably be changed to:
return bind ? bind.getStateCopy(name) : Promise.resolve();

mattwomple · 2019-10-14T18:54:52Z

src/runtime.js

+        return Services.bindForDocOrNull(shadowRoot).then(bind => {
+          return (
+            (bind && bind.setStateAndUpdateHistory(state)) || Promise.resolve()
+          );


This would be more readable as:
return bind ? bind.setStateAndUpdateHistory(state) : Promise.resolve();

dreamofabear

Thanks for taking this on!

dreamofabear · 2019-10-14T18:48:45Z

spec/amp-shadow-doc.md

 - `shadowDoc.postMessage()` and `shadowDoc.onMessage()` - can be used to message with the AMP document.
 - `shadowDoc.close()` - closes the AMP document and frees the resources.
+- `shadowDoc.bind.getState(expr)` - Get an `amp-bind` state from the AMP document using expression syntax
+- `shadowDoc.bind.setState(obj)` - Deep merge an object into the AMP document's global `amp-bind` state


IMO it's fine to not namespace this under .bind since we don't namespace the on="tap:AMP.setState(...)" action either.

dreamofabear · 2019-10-14T18:49:00Z

spec/amp-shadow-doc.md

 - `shadowDoc.setVisibilityState()` - changes the visibility state of the AMP document.
 - `shadowDoc.postMessage()` and `shadowDoc.onMessage()` - can be used to message with the AMP document.
 - `shadowDoc.close()` - closes the AMP document and frees the resources.
+- `shadowDoc.bind.getState(expr)` - Get an `amp-bind` state from the AMP document using expression syntax


Let's mention that expr is a JSON expression string e.g. "foo.bar".

dreamofabear · 2019-10-14T18:49:39Z

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

+   * @param {string} expr
+   * @return {(?JsonObject|string|undefined)}
+   */
+  getStateCopy(expr) {


Nit: Let's just call this getState(). There shouldn't be a use case for returning a non-copy.

dreamofabear · 2019-10-14T18:55:42Z

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

+    // Support expressions
+    if (typeof state === 'string') {
+      // Emulate UIEvent 'click'
+      return this.pushStateWithExpression(


A bit confusing if this API can both "set" and "push" state. I think it's worth separating into two APIs.

Though for the sake of keeping this PR small, can we remove this case?

To make sure I understand, are you suggesting that we do not update history with the shadowDoc.setState() method?

I mean removing this case where passing a string calls "push" instead of "set". The former appends a new entry onto the history stack.

Or maybe you just meant to call setStateWithExpression here?

dreamofabear · 2019-10-14T18:56:38Z

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

+          this.history_.replace(data);
+        }
+      });
+    return this.setStatePromise_;


Would be nice to de-dupe this code snippet with setStateWithExpression. Feel free to create a new private helper method or just drop a TODO(choumx) here for me.

mattwomple · 2019-10-14T23:28:52Z

Thank you for the review! I believe all review comments are resolved in 1a1582e.

dreamofabear

Looks good overall.

/cc @dvoytenko FYI a new shadow API for amp-bind.

dreamofabear · 2019-10-15T19:34:54Z

src/runtime.js

+          // Emulate UIEvent 'click'
+          return bind.setStateWithExpression(
+            /** @type {string} */ (state),
+            /** @type {!JsonObject} */ ({event: 1})


Is {event: 1} useful here?

I haven't dug too deep into how the scope parameter is used, but at least an empty object needs to be passed. I opted to mimic the object that is passed by a single click event here.

Empty object will suffice then. :)

It's used for things like on="submit-success: AMP.setState({foo: event.response})", but there's no real "event context" in this case.

dvoytenko

API LGTM

dreamofabear · 2019-10-16T15:28:09Z

Great contribution!

* PWA: Expose getState and setState Expose shadowDoc.getState and shadowDoc.setState methods for PWAs. * PWA: Remove inconsistent return value for setState * Minor cleanup * Fix check-types * Add micro documentation for PWA (get/set)State * Support PWA setState with expression syntax * Fixup types * Review updates and add tests * Fix check-types * Fix empty string return value from getState * Don't limit return type of getState * Fix unit test * Do not emulate event in setState * Really fix unit test

PWA: Expose getState and setState

8f55d2b

Expose shadowDoc.getState and shadowDoc.setState methods for PWAs.

googlebot added the cla: yes label Oct 10, 2019

amp-owners-bot bot requested a review from dreamofabear October 10, 2019 20:47

mattwomple added 5 commits October 10, 2019 17:18

PWA: Remove inconsistent return value for setState

6cd130c

Minor cleanup

237327c

Fix check-types

55c05d3

Merge remote-tracking branch 'origin/master' into patch-3

644328d

Add micro documentation for PWA (get/set)State

ced02e8

mattwomple commented Oct 11, 2019

View reviewed changes

mattwomple changed the title ~~[WIP] ✨PWA: Expose getState and setState~~ ✨PWA: Expose amp-bind getState and setState Oct 11, 2019

mattwomple marked this pull request as ready for review October 11, 2019 09:13

mattwomple commented Oct 11, 2019

View reviewed changes

extensions/amp-bind/0.1/bind-impl.js Outdated Show resolved Hide resolved

mattwomple added 2 commits October 11, 2019 22:45

Support PWA setState with expression syntax

5d21df4

Fixup types

760c455

mattwomple commented Oct 14, 2019

View reviewed changes

dreamofabear reviewed Oct 14, 2019

View reviewed changes

Review updates and add tests

1a1582e

mattwomple added 4 commits October 14, 2019 16:33

Fix check-types

cc55e03

Fix empty string return value from getState

ffb1fed

Don't limit return type of getState

db49313

Fix unit test

d4e78ef

dreamofabear approved these changes Oct 15, 2019

View reviewed changes

dvoytenko approved these changes Oct 15, 2019

View reviewed changes

mattwomple added 2 commits October 15, 2019 13:16

Do not emulate event in setState

a4b0bb2

Really fix unit test

a30b58e

dreamofabear merged commit 9b45ef8 into ampproject:master Oct 16, 2019

mattwomple deleted the patch-3 branch December 13, 2019 21:34

samouri mentioned this pull request Jan 29, 2020

I2I: "protocol adapters" for client side data massaging. #26474

Closed

Conversation

mattwomple commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattwomple commented Oct 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mattwomple Oct 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dreamofabear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mattwomple commented Oct 14, 2019

Uh oh!

dreamofabear left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dvoytenko left a comment

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Oct 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mattwomple commented Oct 10, 2019 •

edited

Loading

mattwomple commented Oct 10, 2019 •

edited

Loading

mattwomple Oct 14, 2019 •

edited

Loading