Support object literal as AMP.setState() action arg by dreamofabear · Pull Request #7573 · ampproject/amphtml

dreamofabear · 2017-02-15T16:32:19Z

Support new action syntax AMP.setState({'foo': 'bar'})
Parsing of {'foo': 'bar'} is delegated to amp-bind

Context

In amp-bind, bindable state can be mutated via the AMP.setState() action. However, the action-impl and amp-bind parsers have different syntax/implementation. For example, the actions parser currently doesn't support object/array literals, operators, function invocations, etc.

#7399 requests the ability to set nested variables inside bindable state, e.g.

AMP.setState(myAmpState.foo=123)

Proposal

Instead of incrementally adding this functionality to the actions system, this PR adds a new syntax that will delegate parsing of the invocation args to amp-bind's parser:

AMP.setState({myAmpState: {foo: 123}})

Pros:

Avoids increasing complexity and binary size of v0.js for amp-bind-specific functionality
Makes syntax for AMP.setState() consistent with amp-bind parser's syntax

Cons:

Performance hit since amp-bind's parser is slower and more complex
Need to clearly document that {} action syntax is a special case for AMP.setState

Questions

Alternative syntax: on="tap:{foo: bar}?
Should we remove the existing AMP.setState(foo=bar) syntax?
Should we support AMP.setState({foo.bar: 123}) for setting nested state?

dreamofabear · 2017-02-15T16:54:05Z

/to @dvoytenko @kmh287

dvoytenko · 2017-02-16T20:57:38Z

examples/bind/basic.amp.html

  <div>
-    <button on="tap:AMP.setState(foo='foo', isButtonDisabled=true, textClass='redBackground', imgSrc='https://ampbyexample.com/img/Shetland_Sheepdog.jpg', imgSize=200, imgAlt='Sheepdog', videoSrc='https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ForBiggerJoyrides.mp4', videoControls=true)">Click me</button>
+    <button on="tap:AMP.setState(foo='foo', isButtonDisabled=true, textClass='redBackground', imgSrc='https://ampbyexample.com/img/Shetland_Sheepdog.jpg', imgSize=200, imgAlt='Sheepdog', videoSrc='https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ForBiggerJoyrides.mp4', videoControls=true)">Click me (key-value args)</button>
+    <button on="tap:AMP.setState({'foo': 'foo', 'isButtonDisabled': true, 'textClass': 'redBackground', 'imgSrc': 'https://ampbyexample.com/img/Shetland_Sheepdog.jpg', 'imgSize': 200, 'imgAlt': 'Sheepdog', 'videoSrc': 'https://commondatastorage.googleapis.com/gtv-videos-bucket/sample/ForBiggerJoyrides.mp4', 'videoControls': true})">Click me (object args)</button>


What if we stepped away from functional call here completely? I.e.

<button on="tap:{foo: true, ....}">

This is just a suggestion to discuss. But I feel like AMP.setState itself is somewhat awkward. Obviously you may have some additional plans for this, so let's discuss.

IMO it's too ambiguous without AMP.setState, whose name also draws a connection to the <amp-state> component.

/cc @ericlindley-g @kul3r4

I agree with @choumx.

Well, nothing enforces that variables used in amp-bind have to be in amp-state, right?

Does everyone find AMP.setState(a=b) vs AMP.setState({a: b}) clear enough distinction?

Well, nothing enforces that variables used in amp-bind have to be in amp-state, right?

True, but the consistency is nice. Also, AMP.setState is easily searchable in documentation, while {...} is less so.

Does everyone find AMP.setState(a=b) vs AMP.setState({a: b}) clear enough distinction?

I vote yes. 😄

On a somewhat tangent, I wonder if eventually we should remove AMP.setState(a=b) since it has fewer features while looking very similar.

Ok. But could you please bring this up tomorrow for design review? I'd like a wide group to take a look. No need to block this PR on this, however.

Good idea, will do: #7446 (comment)

jridgewell

So I'm just now realizing we can have out-of-order issues with multiple calls to setState.

jridgewell · 2017-02-16T22:11:30Z

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

-        result = binding.expression.evaluate(scope);
-      } catch (error) {
-        errors[expr] = error;
+      const expression = this.expressionCache_[expressionString];


How would we get to this?

Shouldn't happen but guarding against it anyways.

jridgewell · 2017-02-16T22:15:32Z

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

+   *   result: ./bind-expression.BindExpressionResultDef,
+   *   error: Error,
+   * }}
+   */


This is only called when parsing some setState call, correct? Should we really be caching these?

I think we should. A single setState expression will likely be executed several times.

jridgewell · 2017-02-16T22:17:36Z

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

-    }
+    }).then(returnValue => {
+      if (returnValue.error) {
+        user().error(TAG,


Should this reject so the caller knows there was an error?

Good idea, done.

jridgewell · 2017-02-16T22:19:30Z

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

  digest_(opt_verifyOnly) {
    if (this.workerExperimentEnabled_) {
      user().fine(TAG, 'Asking worker to re-evaluate expressions...');
      this.evaluatePromise_ =


Does this need to be set on the instance?

Previously used for tests but not anymore. Done.

jridgewell · 2017-02-16T22:25:26Z

src/service/action-impl.js

+    if (c == OBJECT_SET[0]) { // '{'
+      let end = -1;
+      for (let i = newIndex + 1; i < this.str_.length; i++) {
+        if (this.str_.charAt(i) == OBJECT_SET[1]) { // '}'


Nit, we can just use array index notation instead of #charAt.

jridgewell · 2017-02-16T22:26:20Z

src/service/action-impl.js

+    // Object literal.
+    if (c == OBJECT_SET[0]) { // '{'
+      let end = -1;
+      for (let i = newIndex + 1; i < this.str_.length; i++) {


This fails to consider nested objects.

Yea, that's left to the amp-bind parser. I was conflicted between naming this OBJECT_SET and EXPRESSION_SET, but it technically is an object literal.

What I mean is that { a: {b: 1 } } will cause an error, since we'll start processing this object expression at a, then close it after b. But now we have an extra } character. You'll need to keep track of the number of open {, and only close once we've reached the same number of }.

D'oh, good catch! 👍

jridgewell · 2017-02-16T22:28:03Z

src/service/action-impl.js

   * @param {?Event} event
   */
-  constructor(target, method, args, source, event) {
+  constructor(target, method, args, argsExpression, source, event) {


I'm curious why argsExpression can't just be one of the args?

Considered that but what arg key would we use? There's some risk that a user would stumble upon it and go down an unintended code path.

I suppose we could do something unlikely like __amp_arg_expression. This new param is pretty ugg-o.

I don't believe object-string is a possible key. How about that? If we do use that, we should absolutely add a test that proves the parser will throw trying to use it outside of this object literal codepath.

Changed to use __AMP_OBJECT_LITERAL__, similar to other constants in this file. IMO this is better than object-string since it's clearer to users that this isn't a public API.

If we do use that, we should absolutely add a test that proves the parser will throw trying to use it outside of this object literal codepath.

Not sure we can test this since it's possible for users to create actions like on="target.event(__AMP_OBJECT_LITERAL__=123). Code to enforce this specifically at the parser-level would be even uglier than the previous argsExpression approach due to loss of generality.

Not a huge point, but I thought that argExpression was nice and clear.

kmh287 · 2017-02-17T16:46:08Z

Please resolve merge conflicts.

jridgewell · 2017-02-17T22:36:54Z

src/service/action-impl.js

+    // Object literal.
+    if (c == OBJECT_SET[0]) { // '{'
+      let end = -1;
+      for (let i = newIndex + 1; i < this.str_.length; i++) {


What I mean is that { a: {b: 1 } } will cause an error, since we'll start processing this object expression at a, then close it after b. But now we have an extra } character. You'll need to keep track of the number of open {, and only close once we've reached the same number of }.

jridgewell · 2017-02-17T22:37:18Z

src/service/action-impl.js

+      }
+      const value = this.str_.substring(newIndex, end + 1);
+      newIndex = end;
+      return {type: TokenType.LITERAL, value, index: newIndex};


Maybe a new type, so we don't confuse it for a string?

Thought about that too but technically this is still a "literal". Anyways, done.

jridgewell · 2017-02-17T22:42:29Z

src/service/action-impl.js

   * @param {?Event} event
   */
-  constructor(target, method, args, source, event) {
+  constructor(target, method, args, argsExpression, source, event) {


I don't believe object-string is a possible key. How about that? If we do use that, we should absolutely add a test that proves the parser will throw trying to use it outside of this object literal codepath.

kmh287

Would it be possible to maintain the promises in bind-impl but make them private? The integration tests I'm writing won't call any of those methods directly but will still need to wait on these promises.

dreamofabear · 2017-02-21T16:58:51Z

Would it be possible to maintain the promises in bind-impl but make them private?

@kmh287 I think you might need to make changes anyways due to refactoring. I.e. you'll probably want to use the new initializePromise_ and create a new ivar called setStatePromise_.

dvoytenko · 2017-02-21T18:36:08Z

src/service/standard-actions-impl.js

        bindForDoc(this.ampdoc).then(bind => {
-          bind.setState(invocation.args);
+          const args = invocation.args;
+          const objectString = args[OBJECT_STRING_ARGS_KEY];


I kind of liked a clear separate public field on invocation. Did I miss the thread where you decided to change that?

Discussion thread is here: #7573 (comment)

kmh287 · 2017-02-22T21:57:42Z

Okay, the promise will still need to be exposed for testing, so we'll be trading one promise for another. This will definitely be cleaner though. 👍

dreamofabear · 2017-02-22T23:17:27Z

@dvoytenko To follow-up, React doesn't perform recursive merge -- probably uses Object.assign under the hood. So the current behavior of AMP.setState({...}) will be consistent with React. May be useful in the future though we can discuss later (e.g. AMP.mergeState?).

I also filed #7737 as a follow-up to update documentation for AMP.setState.

@cramforce I'd be happy with <amp-bind-state> and AMP.setBindState(), though they're slightly less nice. And I don't foresee non-bind usages of "AMP state".

cramforce · 2017-02-22T23:37:49Z

Ack

dvoytenko · 2017-02-24T05:10:12Z

@choumx LGTM

jridgewell · 2017-02-24T05:44:16Z

LGTM

* initial commit for exprs in actions * refactor evaluator and clean up * fix type errors * add comments * use separate param for arg expr * more comments * tweak example * unit tests * justin's comments * fix types * fix tests, partially address comments * change arg key to const, add new token type * minor fixes

William Chou added 7 commits February 14, 2017 12:01

initial commit for exprs in actions

4f68576

refactor evaluator and clean up

163f10f

fix type errors

b90bce0

add comments

7f5a78e

use separate param for arg expr

0775dce

more comments

e1a4e0a

tweak example

380e505

dreamofabear added the PR: Work in Progress label Feb 15, 2017

dreamofabear requested a review from dvoytenko February 15, 2017 16:53

dreamofabear mentioned this pull request Feb 15, 2017

Support dot syntax in action arg keys #7465

Closed

unit tests

bdb0fe4

dreamofabear removed the PR: Work in Progress label Feb 15, 2017

dreamofabear requested a review from kmh287 February 15, 2017 17:53

dreamofabear mentioned this pull request Feb 15, 2017

amp-bind: Only primitives for object literal keys #7582

Merged

dvoytenko reviewed Feb 16, 2017

View reviewed changes

jridgewell reviewed Feb 16, 2017

View reviewed changes

kul3r4 mentioned this pull request Feb 17, 2017

Request: additional examples for amp-bind (amp-video[src] + XHR) ampproject/amp-by-example#574

Closed

dreamofabear mentioned this pull request Feb 17, 2017

Text and Amp-Carousel Bind Integration Tests #7637

Merged

Merge branch 'master' into amp-set-state-expr

8cfa7c3

dreamofabear force-pushed the amp-set-state-expr branch from e08436b to 3797208 Compare February 17, 2017 22:23

justin's comments

cd71d8d

dreamofabear force-pushed the amp-set-state-expr branch from 3797208 to cd71d8d Compare February 17, 2017 22:27

jridgewell reviewed Feb 17, 2017

View reviewed changes

William Chou added 3 commits February 17, 2017 17:46

fix types

fc9fda2

fix tests, partially address comments

4245c41

change arg key to const, add new token type

25dc4c3

kmh287 requested changes Feb 21, 2017

View reviewed changes

dreamofabear mentioned this pull request Feb 21, 2017

Weekly Status 2017-02-20 #7679

Closed

dvoytenko reviewed Feb 21, 2017

View reviewed changes

dreamofabear mentioned this pull request Feb 21, 2017

Design Review 2017-02-22 #7446

Closed

kmh287 approved these changes Feb 22, 2017

View reviewed changes

dreamofabear mentioned this pull request Feb 23, 2017

amp-bind: Turing machine and game examples #7742

Merged

minor fixes

5d05aed

dreamofabear force-pushed the amp-set-state-expr branch from b5353a1 to 5d05aed Compare February 23, 2017 16:36

dreamofabear merged commit dd15659 into ampproject:master Feb 24, 2017

dreamofabear deleted the amp-set-state-expr branch February 24, 2017 16:48

dreamofabear mentioned this pull request Feb 28, 2017

Weekly Status 2017-02-27 #7813

Closed

kul3r4 mentioned this pull request Mar 1, 2017

Update amp-bind sample with new setState syntax ampproject/amp-by-example#669

Closed

Conversation

dreamofabear commented Feb 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Proposal

Questions

Uh oh!

dreamofabear commented Feb 15, 2017 • 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!

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!

jridgewell 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmh287 commented Feb 17, 2017

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!

kmh287 left a comment

Choose a reason for hiding this comment

Uh oh!

dreamofabear commented Feb 15, 2017 •

edited

Loading

dreamofabear commented Feb 15, 2017 •

edited

Loading