Support action chaining (for multiple actions in a single event) #8678

dreamofabear · 2017-04-10T20:53:21Z

Fixes #10914.

Add a {?Promise} return type to action handlers.
If an action handler returns a Promise, subsequent actions triggered by the same event will wait for the Promise to resolve before invocation.
Only allow a single AMP.setState per action "chain" to sidestep performance constraint issue (for now).

Related: #8648 (comment).

edhollinghurst · 2017-06-12T15:12:42Z

I have a vested interest in getting this in for a project I'm working on. Is there anything I can do to help?

@bjalford FYI

dreamofabear · 2017-06-12T19:41:58Z

@edhollinghurst Would you mind describing your use case? This PR was exploratory in nature.

edhollinghurst · 2017-06-12T19:58:50Z

@choumx Yes I'm just to use amp-bind to set the field values on a hidden form before triggering it to submit.

Basically the first scenario that you describe here: #8648 (comment)

dreamofabear · 2017-06-12T20:02:04Z

@edhollinghurst Thanks. Does using amp-state with a [src] binding not fit your use case (the second suggestion on the same comment)?

edhollinghurst · 2017-06-12T21:01:34Z

@choumx Thanks. I'm not sure it will work for my use case though as I'm looking to POST to the backend.

I've created an example here: https://www.edhollinghurst.com/amp-bind-voting/amp-bind-voting.html

For this example I've not actually set up a backend for the form to post to but hopefully you get the idea of what I'm trying to achieve.

dreamofabear · 2017-08-17T21:38:15Z

/to @alanorozco /cc @dvoytenko

dreamofabear · 2017-08-22T21:39:14Z

@alanorozco Friendly ping.

jridgewell · 2017-08-22T22:03:03Z

src/service/action-impl.js

+
+      // Wait for the previous action, if applicable.
+      currentPromise = (currentPromise)
+          ? currentPromise.then(() => invoke())


Nit: no need for a second closure here.

jridgewell · 2017-08-22T22:05:36Z

src/service/action-impl.js

+      // Wait for the previous action, if applicable.
+      currentPromise = (currentPromise)
+          ? currentPromise.then(() => invoke())
+          : invoke();


This might return null, right?

Yes. All actions up to the first one that returns a Promise will execute synchronously.

What I mean is that the first promise might actually be null, and doing null.then is an error.

If invoke() returns null, then during the next iteration it'll fall into the "else" branch of the ternary again, no? Note that currentPromise is initialized to null.

jridgewell · 2017-08-22T22:07:19Z

src/service/standard-actions-impl.js

+        // Only allow one AMP.setState action per event.
+        const actionInfos =
+            /** @type {!Array} */ (dev().assert(opt_actionInfos));
+        const firstSetState = findIndex(actionInfos, actionInfo => {


Nit: use a for loop and limit to opt_actionIndex

jridgewell · 2017-08-23T19:37:40Z

src/service/action-impl.js

@@ -392,7 +392,7 @@ export class ActionService {

      // Wait for the previous action, if applicable.
      currentPromise = (currentPromise)
-          ? currentPromise.then(() => invoke())
+          ? currentPromise.then(invoke())


Not quite. You want to pass the invoke function, not call it. 😛

jridgewell · 2017-08-23T19:38:15Z

src/service/action-impl.js

+      // Wait for the previous action, if applicable.
+      currentPromise = (currentPromise)
+          ? currentPromise.then(() => invoke())
+          : invoke();


What I mean is that the first promise might actually be null, and doing null.then is an error.

cvializ · 2017-08-24T19:02:35Z

Do you have an example of the syntax for chaining actions? Looking at the source it's not obvious what it might look like in the markup.

dreamofabear · 2017-08-24T20:12:47Z

@cvializ There's no new syntax, just a behavior change. For example:

<!-- #form1 will wait for AMP.setState to complete before invoking `submit`. -->
<button on="tap:AMP.setState({x: 123}), form1.submit">

The link on OP has more usage context.

dreamofabear · 2017-08-25T17:25:07Z

@jridgewell Any other comments?

edhollinghurst · 2017-08-31T08:51:07Z

Thanks for this @choumx !

dreamofabear added the PR use: Work in Progress label Apr 10, 2017

ampwafflebot added the in progress label Apr 10, 2017

dreamofabear mentioned this pull request Apr 10, 2017

amp-bind: Support e-commerce XHR use case #8648

Closed

dreamofabear changed the title ~~amp-bind: Actions after setState will wait for its completion~~ [WIP] Actions after AMP.setState will wait for its completion Apr 12, 2017

kmh287 mentioned this pull request Apr 25, 2017

amp-bind: Support intermediary vars in AMP.setState() #8397

Closed

dreamofabear mentioned this pull request Apr 25, 2017

amp-bind: Cycles with amp-state's [src] binding #8948

Closed

googlebot added the cla: yes label Jun 12, 2017

dreamofabear mentioned this pull request Aug 15, 2017

amp-bind ordering #10914

Closed

dreamofabear changed the title ~~[WIP] Actions after AMP.setState will wait for its completion~~ [Experimental] Actions after AMP.setState will wait for its completion Aug 15, 2017

dreamofabear force-pushed the bind-ecommerce-xhr branch from 74c146c to 74ceb53 Compare August 15, 2017 16:20

dreamofabear changed the title ~~[Experimental] Actions after AMP.setState will wait for its completion~~ [Experimental] Actions after AMP.setState will wait for its completion Aug 15, 2017

dreamofabear force-pushed the bind-ecommerce-xhr branch 2 times, most recently from fe275f1 to c419788 Compare August 15, 2017 23:09

dreamofabear changed the title ~~[Experimental] Actions after AMP.setState will wait for its completion~~ Support action chaining (for multiple actions in a single event) Aug 17, 2017

dreamofabear removed the PR use: Work in Progress label Aug 17, 2017

dreamofabear requested a review from alanorozco August 17, 2017 21:38

jridgewell requested changes Aug 22, 2017

View reviewed changes

dreamofabear requested review from aghassemi, cvializ and dvoytenko as code owners August 23, 2017 19:35

jridgewell reviewed Aug 23, 2017

View reviewed changes

William Chou added 10 commits August 24, 2017 17:13

squash and isolate action chaining commits

54548b0

update action handlers, support global method handlers

7cd23ae

add unit test

0830de9

lint fix

8b5b357

only allow one amp-bind action per event, minor refactor

99fe9a0

move setState() chain restriction to standard-actions-impl.js

fac568c

add unit test

7b829ef

fix tests

8da5ebe

add comment

c16cdd5

justin's comments

cc56937

dreamofabear force-pushed the bind-ecommerce-xhr branch from b430566 to 9a7bead Compare August 24, 2017 21:45

fix merge errors

8f518ac

dreamofabear force-pushed the bind-ecommerce-xhr branch from 9a7bead to 8f518ac Compare August 25, 2017 17:24

jridgewell approved these changes Aug 25, 2017

View reviewed changes

dreamofabear merged commit 4788fd9 into ampproject:master Aug 25, 2017

dreamofabear deleted the bind-ecommerce-xhr branch August 25, 2017 17:58

dreamofabear mentioned this pull request Aug 30, 2017

Limit history changes in AMP? #11071

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support action chaining (for multiple actions in a single event) #8678

Support action chaining (for multiple actions in a single event) #8678

dreamofabear commented Apr 10, 2017 •

edited

edhollinghurst commented Jun 12, 2017

dreamofabear commented Jun 12, 2017

edhollinghurst commented Jun 12, 2017

dreamofabear commented Jun 12, 2017

edhollinghurst commented Jun 12, 2017

dreamofabear commented Aug 17, 2017

dreamofabear commented Aug 22, 2017

jridgewell Aug 22, 2017

dreamofabear Aug 23, 2017

jridgewell Aug 22, 2017

dreamofabear Aug 23, 2017

jridgewell Aug 23, 2017

dreamofabear Aug 23, 2017

jridgewell Aug 22, 2017

dreamofabear Aug 23, 2017

jridgewell Aug 23, 2017

dreamofabear Aug 23, 2017

jridgewell Aug 23, 2017

cvializ commented Aug 24, 2017

dreamofabear commented Aug 24, 2017

dreamofabear commented Aug 25, 2017

edhollinghurst commented Aug 31, 2017

Support action chaining (for multiple actions in a single event) #8678

Support action chaining (for multiple actions in a single event) #8678

Conversation

dreamofabear commented Apr 10, 2017 • edited

edhollinghurst commented Jun 12, 2017

dreamofabear commented Jun 12, 2017

edhollinghurst commented Jun 12, 2017

dreamofabear commented Jun 12, 2017

edhollinghurst commented Jun 12, 2017

dreamofabear commented Aug 17, 2017

dreamofabear commented Aug 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cvializ commented Aug 24, 2017

dreamofabear commented Aug 24, 2017

dreamofabear commented Aug 25, 2017

edhollinghurst commented Aug 31, 2017

dreamofabear commented Apr 10, 2017 •

edited