Skip to content

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

Merged
dreamofabear merged 2 commits intoampproject:masterfrom
dreamofabear:no-exprs-in-keys
Feb 21, 2017
Merged

amp-bind: Only primitives for object literal keys #7582
dreamofabear merged 2 commits intoampproject:masterfrom
dreamofabear:no-exprs-in-keys

Conversation

@dreamofabear
Copy link
Copy Markdown

@dreamofabear dreamofabear commented Feb 15, 2017

Related to #7399.

  • Only allow primitives as object literal keys, e.g. don't allow {1+1: 'abc'}.

Originally thought "why not", but realized this will be confusing for users once we support object literals in AMP.setState() (#7573).

Specifically, users will probably conflate

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

with

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

when actually it will evaluate to current value of foo.bar.

Separately, perhaps we should support the above behavior for user convenience. Will follow up in a separate PR since that's a larger change.

/to @jridgewell @kmh287 /cc @kul3r4

@dreamofabear dreamofabear changed the title amp-bind: Only strings for object literal keys amp-bind: Only primitive literals for object literal keys Feb 15, 2017
@dreamofabear dreamofabear changed the title amp-bind: Only primitive literals for object literal keys amp-bind: Only primitives for object literal keys Feb 15, 2017
Copy link
Copy Markdown
Contributor

@kmh287 kmh287 left a comment

Choose a reason for hiding this comment

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

LGTM

@dreamofabear dreamofabear merged commit b330812 into ampproject:master Feb 21, 2017
@dreamofabear dreamofabear deleted the no-exprs-in-keys branch February 21, 2017 17:05
mrjoro pushed a commit to mrjoro/amphtml that referenced this pull request Apr 28, 2017
* initial commit to forbid exprs in obj keys

* also support numbers and other primitives
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.

3 participants