Skip to content

Consider annotator differentiating between [[3, 4]] and [[binding]] #1734

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

Closed
sjmiles opened this issue Jun 3, 2015 · 10 comments · Fixed by #4768
Closed

Consider annotator differentiating between [[3, 4]] and [[binding]] #1734

sjmiles opened this issue Jun 3, 2015 · 10 comments · Fixed by #4768

Comments

@sjmiles
Copy link
Contributor

sjmiles commented Jun 3, 2015

Currently [[ characters trigger binding logic immediately, even if the expression is actually JSON data.

@atotic
Copy link
Contributor

atotic commented Jun 3, 2015

Why is there no warning when binding logic encounters: '[["Jan", 31],["Feb", 28],["Mar", 31]]'. It is not parseable as a binding, the code must swallow an error quietly somewhere.

@zikes
Copy link

zikes commented Jun 4, 2015

I think I found the offending check at https://github.com/Polymer/polymer/blob/master/src/lib/annotations/annotations.html#L93

I think the fix would be to check that the third character is either an underscore or a letter, as those are the only valid starting characters for bindable properties or computable functions, right?

@SmokyBob
Copy link

SmokyBob commented Jun 4, 2015

@zikes I think checking the 3rd character is not enough, for example

'[[true, "Permission1"],[false, "Permission2"]]'

Will still fail to work properly.
Perhaps, checking the presence of a comma in the remaining text might be enough?
Trying to JSON.parse the remaining string I think would be the safes, but I think it's too detrimental for performance

@zikes
Copy link

zikes commented Jun 4, 2015

@SmokyBob I forgot about booleans, that throws a real wrench into that idea. Unfortunately that also rules out the comma check, as [[true]] is still valid JSON.

I think a JSON.parse may actually be necessary, but could be a last-resort check after anything else that might be more certain and performant.

@zikes
Copy link

zikes commented Jun 4, 2015

I just remembered that computed annotations can have multiple parameters, which would also contain commas.

@wq9
Copy link

wq9 commented Jun 4, 2015

You can solve this by adding a space between [[. For example,
attribute='[ ["Jan", 31],["Feb", 28],["Mar", 31] ]'
Now, attribute contains an array instead of data binding.

The mistake was using [[ for data binding because it collides with array of array syntax. Consider using {[ next time.

@SmokyBob
Copy link

SmokyBob commented Jun 4, 2015

@zikes you are right, forgot about that computed properties can have multiple values (or an array as the parameter).

I like @wq9 idea of using {[ for the one way binding, but it will not solve the issue; I can pass an object as parameter where the first item is an array.

@zikes
Copy link

zikes commented Jun 4, 2015

{[ is not valid JSON notation as the property name would have to come first.

Unfortunately {[ is a very breaking change. Even if there is a significant performance hit, better to JSON.parse first and try to optimize from there.

@SmokyBob
Copy link

SmokyBob commented Jun 4, 2015

@zikes got me :) answering before coffee was not a good idea.

{[ is indeed a very breaking change... but (IMO) it's better to make this small breaking change now when the project is still fresh and we are starting to learn the new binding system than in 6 months and have people yelling...
maybe sneaking in this change with the release of the literal binding support
(ex. <div>my text {{prop}}</div>)
would make the pill sweeter to swallow

@TimvdLippe
Copy link
Contributor

This issue is fixed as of #3017

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

Successfully merging a pull request may close this issue.

8 participants