New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support object literal as AMP.setState() action arg #7573
Support object literal as AMP.setState() action arg #7573
Conversation
/to @dvoytenko @kmh287 |
@@ -34,7 +34,8 @@ | |||
<p>The image above will increase in size and change its src</p> | |||
<amp-video src="https://ampbyexample.com/video/tokyo.mp4" [src]="videoSrc" width=480 height=270 autoplay [controls]="videoControls"></amp-video> | |||
<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> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it's too ambiguous without AMP.setState
, whose name also draws a connection to the <amp-state>
component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with @choumx.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, nothing enforces that variables used in amp-bind
have to be in amp-state
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does everyone find AMP.setState(a=b)
vs AMP.setState({a: b})
clear enough distinction?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, will do: #7446 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I'm just now realizing we can have out-of-order issues with multiple calls to setState
.
} catch (error) { | ||
errors[expr] = error; | ||
const expression = this.expressionCache_[expressionString]; | ||
if (!expression) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would we get to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't happen but guarding against it anyways.
* error: Error, | ||
* }} | ||
*/ | ||
evaluateExpression(expressionString, scope) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only called when parsing some setState
call, correct? Should we really be caching these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should. A single setState
expression will likely be executed several times.
} | ||
}).then(returnValue => { | ||
if (returnValue.error) { | ||
user().error(TAG, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this reject so the caller knows there was an error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, done.
extensions/amp-bind/0.1/bind-impl.js
Outdated
@@ -320,19 +343,20 @@ export class Bind { | |||
* If `opt_verifyOnly` is true, does not apply results but verifies them | |||
* against current element values instead. | |||
* @param {boolean=} opt_verifyOnly | |||
* @return {!Promise} | |||
* @private | |||
*/ | |||
digest_(opt_verifyOnly) { | |||
if (this.workerExperimentEnabled_) { | |||
user().fine(TAG, 'Asking worker to re-evaluate expressions...'); | |||
this.evaluatePromise_ = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to be set on the instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously used for tests but not anymore. Done.
src/service/action-impl.js
Outdated
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]) { // '}' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, we can just use array index notation instead of #charAt
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
src/service/action-impl.js
Outdated
// Object literal. | ||
if (c == OBJECT_SET[0]) { // '{' | ||
let end = -1; | ||
for (let i = newIndex + 1; i < this.str_.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails to consider nested objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh, good catch! 👍
src/service/action-impl.js
Outdated
* @param {?Element} source | ||
* @param {?Event} event | ||
*/ | ||
constructor(target, method, args, source, event) { | ||
constructor(target, method, args, argsExpression, source, event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why argsExpression
can't just be one of the args
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a huge point, but I thought that argExpression
was nice and clear.
Please resolve merge conflicts. |
e08436b
to
3797208
Compare
3797208
to
cd71d8d
Compare
src/service/action-impl.js
Outdated
// Object literal. | ||
if (c == OBJECT_SET[0]) { // '{' | ||
let end = -1; | ||
for (let i = newIndex + 1; i < this.str_.length; i++) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 }
.
src/service/action-impl.js
Outdated
} | ||
const value = this.str_.substring(newIndex, end + 1); | ||
newIndex = end; | ||
return {type: TokenType.LITERAL, value, index: newIndex}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a new type, so we don't confuse it for a string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thought about that too but technically this is still a "literal". Anyways, done.
src/service/action-impl.js
Outdated
* @param {?Element} source | ||
* @param {?Event} event | ||
*/ | ||
constructor(target, method, args, source, event) { | ||
constructor(target, method, args, argsExpression, source, event) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@kmh287 I think you might need to make changes anyways due to refactoring. I.e. you'll probably want to use the new |
@@ -71,7 +71,20 @@ export class StandardActions { | |||
switch (invocation.method) { | |||
case 'setState': | |||
bindForDoc(this.ampdoc).then(bind => { | |||
bind.setState(invocation.args); | |||
const args = invocation.args; | |||
const objectString = args[OBJECT_STRING_ARGS_KEY]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of liked a clear separate public field on invocation
. Did I miss the thread where you decided to change that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussion thread is here: #7573 (comment)
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. 👍 |
@dvoytenko To follow-up, React doesn't perform recursive merge -- probably uses I also filed #7737 as a follow-up to update documentation for @cramforce I'd be happy with |
Ack |
b5353a1
to
5d05aed
Compare
@choumx LGTM |
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
Fixes #7399.
AMP.setState({'foo': 'bar'})
{'foo': 'bar'}
is delegated toamp-bind
Context
In
amp-bind
, bindable state can be mutated via theAMP.setState()
action. However, theaction-impl
andamp-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.
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:Pros:
amp-bind
-specific functionalityAMP.setState()
consistent withamp-bind
parser's syntaxCons:
amp-bind
's parser is slower and more complex{}
action syntax is a special case forAMP.setState
Questions
on="tap:{foo: bar}
?AMP.setState(foo=bar)
syntax?AMP.setState({foo.bar: 123})
for setting nested state?