Skip to content
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

Add variable references in action method arg values #6723

Merged
merged 6 commits into from
Dec 22, 2016

Conversation

dreamofabear
Copy link

@dreamofabear dreamofabear commented Dec 16, 2016

Partial for #6199.

This allows method calls to reference data in the Event object that triggered them. For example:

<component on="myEvent:myTarget.myMethod(key=event.foo)">

If myEvent is triggered with a CustomEvent.detail = {foo: 'bar'}, then myMethod() will be called with args {key: 'bar'}.

@dreamofabear
Copy link
Author

/to @dvoytenko PTAL!

@@ -628,7 +690,7 @@ class ParserTokenizer {
const value = convertValues && (s == 'true' || s == 'false') ?
s == 'true' : s;
newIndex = end - 1;
return {type: TokenType.LITERAL, value, index: newIndex};
return {type: TokenType.ID, value, index: newIndex};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like if it's true or false - this should still be LITERAL, right? A literal boolean value? Since we likewise have literal numeric values....

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, ID probably isn't a good name. I wasn't sure how JS-like we should make this since non-quoted strings are currently treated as string literals.

Changed so that true, false and numerics won't be classified as ID.

});
return applied;
}
return args;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move on top as if (!args) {return args;}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

const value = token.value;
if (token.type == TokenType.LITERAL) {
return () => value;
} else if (token.type == TokenType.ID) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, this changes the spec in described in the https://github.com/ampproject/amphtml/blob/master/spec/amp-html-format.md#on

Looks like the docs should be changed. How compatible are we with the current doc?

Looks like we fallback to the literal value. Would that look strange per doc?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like the docs should be changed. How compatible are we with the current doc?

I was planning to wait until at least one component supports exposing event data before updating the docs. There's no viable use case yet.

Looks like we fallback to the literal value. Would that look strange per doc?

Agreed. As discussed, I changed the format to allow dereferencing only through . operator in argument values, e.g. on="event:method(key=event.variable).

@dreamofabear dreamofabear merged commit 22290b7 into ampproject:master Dec 22, 2016
torch2424 pushed a commit to torch2424/amphtml that referenced this pull request Jan 3, 2017
* implement var dereferencing in action arg values

* fix type error

* add unit tests for applyActionInfoArgs()

* fix lint error

* PR comments

* fix lint errors
@dreamofabear dreamofabear deleted the action-variables branch January 9, 2017 22:27
jridgewell pushed a commit to jridgewell/amphtml that referenced this pull request Jan 31, 2017
* implement var dereferencing in action arg values

* fix type error

* add unit tests for applyActionInfoArgs()

* fix lint error

* PR comments

* fix lint errors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants