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

Fix compound bindings with braces in literals #2705

Merged

Conversation

TimvdLippe
Copy link
Contributor

The problem in this regex was the [^}\]]* which also triggers on a single }. Therefore we have to check for a group of two } or ]. This is now changed to a negative look-ahead ?! which checks for \}\} and \]\]. After that the rest of the matches are non-greedy, to prevent double consuming of {{binding1}}{{binding2}} to be translated into a binding with variable name binding1}}{{binding2.

I also escaped all } and ] to force matching on literal characters.

image

Fixes #2704

@samccone
Copy link
Contributor

samccone commented Nov 9, 2015

wow.. you are too fast @JeremybellEU :)

@samccone
Copy link
Contributor

samccone commented Nov 9, 2015

cc @kevinpschaaf @ebidel for 👀

@kevinpschaaf
Copy link
Member

LGTM 👍

kevinpschaaf added a commit that referenced this pull request Nov 9, 2015
…es-fix

Fix compound bindings with braces in literals
@kevinpschaaf kevinpschaaf merged commit a2c51bf into Polymer:master Nov 9, 2015
@TimvdLippe TimvdLippe deleted the compound-binding-with-braces-fix branch November 9, 2015 17:54
@david-saslawsky
Copy link

@JeremybellEU many thanks for the fix.

Shouldn't the same solution be applied to the ([^{[]*) at the beginning ? Not very likely I know, but I'm thinking of cases like this one:

<span>[YES]/[NO] {{computeCompound('world', 'username ', 'Hello {0} ')}}</span>

Also, how does the negative lookahead works ? I might be missing something but it seems to me that the regexp works fine without it because of the non greedy match on (.+?):

/([^{[]*)(\{\{|\[\[)(.+?)(?:\]\]|\}\})/g

Anyway, thanks again. I can remove ugly workarounds in my application now 👍

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

Successfully merging this pull request may close these issues.

5 participants