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

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

Closed
dreamofabear opened this issue Mar 24, 2017 · 9 comments
Closed

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

dreamofabear opened this issue Mar 24, 2017 · 9 comments

Comments

@dreamofabear
Copy link

dreamofabear commented Mar 24, 2017

Benefits

  • Reduces document markup due to code duplication.
  • Improves performance by caching expression fragments.

Possible solution

One approach is to allow chained actions. E.g. instead of...

on="tap:AMP.setState({foo: 1 + 2, bar: (1 + 2) + 3})"

...do this:

on="tap:AMP.setState({onePlusTwo: 1 + 2}), 
        AMP.setState({foo: onePlusTwo, bar: onePlusTwo + 3})"

This doesn't work today because the second action doesn't wait for the first to complete.

Only really painful in more complicated use cases though, e.g. examples/bind/turing.amp.html.

@kmh287
Copy link
Contributor

kmh287 commented Mar 27, 2017

Could setState take a variable number of object literals and then call bind's setState on them in order?

@kmh287
Copy link
Contributor

kmh287 commented Mar 28, 2017

Also, couldn't this be used to get around our expression complexity limit?

@dreamofabear
Copy link
Author

Could setState take a variable number of object literals and then call bind's setState on them in order?

Yes, though it's a slightly more complex change since we'd have to change the action parser.

Also, couldn't this be used to get around our expression complexity limit?

Expressions in setState should go through the same code path.

@kmh287
Copy link
Contributor

kmh287 commented Mar 28, 2017

I'm thinking though that if we allow multi-step calculations, then someone could just break up a larger calculation into multiple steps to circumvent the complexity limit.

For instance if my expression is A + B where A and B are complex expressions and (A + B) has an AST size of 51 (one over the current limit) then I could do.

AMP.setState({x: A, y: B}), AMP.setState({z: x + y})

or

AMP.setState({x:A}), AMP.setState({y:B}), AMP.setState({z:x+y})

@dreamofabear
Copy link
Author

Good point, we'd have to have a more sophisticated constraint to avoid that.

@dreamofabear dreamofabear removed their assignment Apr 5, 2017
@kmh287
Copy link
Contributor

kmh287 commented Apr 5, 2017

We could put a limit on the number of sequential setStates, but even then each one we allow multiplies our effective complexity limit.

@dreamofabear
Copy link
Author

Another option is to add a new custom element:

<amp-variable id="onePlusOne" expression="1 + 1">

Slightly less powerful and more explicit than chaining AMP.setState actions.

@kmh287
Copy link
Contributor

kmh287 commented Apr 25, 2017

Once #8678 is in, won't users be able to chain setState to create intermediary vars?

@dreamofabear
Copy link
Author

Fixed with #12285.

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

No branches or pull requests

3 participants