Skip to content

Add variable references in action method arg values#6723

Merged
dreamofabear merged 6 commits intoampproject:masterfrom
dreamofabear:action-variables
Dec 22, 2016
Merged

Add variable references in action method arg values#6723
dreamofabear merged 6 commits intoampproject:masterfrom
dreamofabear:action-variables

Conversation

@dreamofabear
Copy link
Copy Markdown

@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
Copy Markdown
Author

/to @dvoytenko PTAL!

const value = convertValues && (s == 'true' || s == 'false') ?
s == 'true' : s;
newIndex = end - 1;
return {type: TokenType.LITERAL, value, index: newIndex};
Copy link
Copy Markdown
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
Copy Markdown
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.

Object.keys(args).forEach(key => {
applied[key] = args[key].call(null, data);
});
return applied;
Copy link
Copy Markdown
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done.

*/
function getActionInfoArgValue(token) {
const value = token.value;
if (token.type == TokenType.LITERAL) {
Copy link
Copy Markdown
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
Copy Markdown
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