Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix($interpolate): always unescape escaped interpolation markers #14199

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

gkalpak
Copy link
Member

@gkalpak gkalpak commented Mar 8, 2016

Previously, whenever mustHaveExpression was true (e.g. when compiling a text node),
$interpolate would not unescape the escaped interpolation markers if there were no actual
interpolation expressions in the same string.
This commit fixes the problem, by always unescaping the escaped markers (if any).

Due to always checking for the presence of escaped interpolation markers, there is a slight
performance hit.

Fixes #14196

Previously, whenever `mustHaveExpression` was true (e.g. when compiling a text node),
`$interpolate` would not unescape the escaped interpolation markers if there were no actual
interpolation expressions in the same string.
This commit fixes the problem, by always unescaping the escaped markers (if any).

Due to always checking for the presence of escaped interpolation markers, there is a slight
performance hit.

Fixes angular#14196
@gkalpak
Copy link
Member Author

gkalpak commented Mar 8, 2016

I wonder if this is a BC.

Possible breakages:

  1. Even if there are no actual interpolation expressions, interpolating strings with escaped markers will return an interpolation function. Previously it would return undefined (and not unescape the escaped markers).
  2. If you relied on the current (documented!) behavior, of not unescaping escaped markers on strings with no actual interpolation expressions.

@lgalfaso
Copy link
Contributor

lgalfaso commented Mar 9, 2016

This PR brings a new level of complexity when people want to have the
literal of value "\{\}" as you would need to write \\{\\{, this is \
should be escaped but only if it would get confused with an interpolation.
This is, it is not possible to write a context free function that takes one
char at a time and returns the escaped char.

This is, we should unescape everything (not only \{) or keep the current
behavior. The former would be a BC.

On Tuesday, March 8, 2016, Georgios Kalpakas notifications@github.com
wrote:

I wonder if this is a BC.

Possible breakages:

  1. Even if there are no actual interpolation expressions,
    interpolating strings with escaped markers will return an interpolation
    function. Previously it would return undefined (and not unescape the
    escaped markers).
  2. If you relied on the current (documented!) behavior, of not
    unescaping escaped markers on strings with no actual interpolation
    expressions.


Reply to this email directly or view it on GitHub
#14199 (comment).

@gkalpak
Copy link
Member Author

gkalpak commented Mar 9, 2016

Maybe I didn't understand well, but with the current behavior, \{\{ might or might not be unescaped depending on whether an actual intetpolation exists in the same string. Doesn't it mean that a context free function won't work now either ?

@lgalfaso
Copy link
Contributor

lgalfaso commented Mar 9, 2016

@gkalpak agree, but this PR moves the issue from one pattern to another :)

@petebacondarwin
Copy link
Member

I think this looks good to me. The issue that @lgalfaso points out is already a problem for text blocks that contain interpolation expressions anyway. This just brings consistency between blocks that do and do not contain interpolation expressions.

@petebacondarwin
Copy link
Member

Breaking change? Well at the moment I would argue that the escaping is broken, since it is inconsistent and I doubt that anyone is really relying on these escaped sequences not being unescaped in text blocks that do not contain interpolation expressions.

@lgalfaso
Copy link
Contributor

lgalfaso commented Mar 9, 2016

@petebacondarwin if this PR lands, how would you write \{\{?

@petebacondarwin
Copy link
Member

I am not sure. But however you do it after this PR it will be the same as you would do it before the PR, in the case that there was some interpolated expression in the same text block.

One ugly way would be to do...

{{ "\\{\\{" }}

@lgalfaso
Copy link
Contributor

lgalfaso commented Mar 9, 2016

I think everybody agrees that the current is less than ideal. I would not
call it broken, but find it normal that others do think it is broken. Now,
this is what I find strange. We know that there are some limitation and
this PR removes some of them, but at the same time it adds new limitations
that worked fine before.

On Wednesday, March 9, 2016, Pete Bacon Darwin notifications@github.com
wrote:

I am not sure. But however you do it after this PR it will be the same as
you would do it before the PR, in the case that there was some interpolated
expression in the same text block.

One ugly way would be to do...

{{ "{{" }}


Reply to this email directly or view it on GitHub
#14199 (comment).

@petebacondarwin
Copy link
Member

The new limitation that worked before was that this PR fixes the inconsistency that if a text node did not contain any interpolations then we would not try to unescape our publicly documented escape sequence for curly braces.

I would say that this was such a weird inconsistency that no one could have been using the escaping effectively, since their server-side escaping mechanism must have been able to ascertain whether a set of double curlies, which were supposed to be rendered as-is, appeared in a text block that actually contained a set of double curlies, which were supposed to be interpolated.

I agree that we need a clear way of escaping the escape sequence.

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

Successfully merging this pull request may close these issues.

Escaped interpolation markers are not unescaped if there are no interpolated expressions in the same text
4 participants