fix(interpolate): changes the interpolate function to escape double quotes

[vsavkin]

Fix a bug in the interpolate function causing it not to handle expressions containing double quotes properly.

Closes #937

[vicb]

Missing a return type.
It would also miss with already escaped string, right ?

[vsavkin]

I'll the return type. Could you provide an example when an escaped string would cause a problem?

[vicb]

@vsavkin many thanks for that. I've added some inline comments.

[vicb]

Could https://api.dartlang.org/apidocs/channels/stable/dartdoc-viewer/dart-convert.HtmlEscape be helpful here ?

[vsavkin]

@vicb I looked into HtmlEscape, but I don't think it can be used here. It escapes quotes using html escaping, so "{{true}}" will become &quottrue$quot. Also, I think we want to keep html stuff as it is (so '
' remains '
').

[vicb]

I'm not sure regexp would be a good fit either... my best guess would be manual parsing when the string contains escape-able chars ?

[vsavkin]

Sure, I can use manual parsing. Do you prefer manual parsing just for clarity or do you see any cases where regexp won't handle the input properly?

[vicb]

May be it's only being too conservative here ??

What if you are in a JS context then \" and \\\" would not be equivalent ?

[vsavkin]

Sorry, I'm not sure if I understand. Looking at how Interpolator is being used, it looks like it is supposed to return an expression that can be evaluated. For example:

console.log(""") prints " // 1 quote character.

interpolate(r'console.log(""")') returns "console.log("\"")"
and eval("console.log("\"")") prints " // 1 quote character.

It seems that interpolating a piece of js code and then evaluating it produces the same result as just executing it.

Is my understand of the responsibilities of Interpolator correct? Could you provide an example showing the issue you are talking about?

[vicb]

A use case could be

  <input type="text" ng-model="ctrl.name">
  <script>
    setInterval(function(){console.log("Hello")}, 3000);
    // {{ ctrl.name }} - to activate interp
  </script>

Not sure if this one is really relevant and if more relevant UC can be found...

[vicb]

It think adding the quotes ("") in _escapeQuote() (which should be renamed) would be more efficient (ie not need to parse the string, direct function call.

[vsavkin]

Are you suggesting something like:

String _wrapInQuotes(String s){
final escaped = s.replaceAll(r'', r'').replaceAll(r'"', r'"');
return '"${escaped}"';
}

[vicb]

Yep '"$escaped"' would be fine.

On 04/23/2014 07:34 PM, Victor Savkin wrote:

In lib/core/interpolate.dart:

@@ -49,7 +49,7 @@ class Interpolate implements Function {
if (index < startIdx) {
// Empty strings could be stripped thanks to the stringify
// formatter

•      expParts.add('"${template.substring(index, startIdx)}"');
  

•      expParts.add('"${_escapeQuotes(template.substring(index, startIdx))}"');
  

Are you suggesting?

String _wrapInQuotes(String s){
final escaped = s.replaceAll(r'', r'').replaceAll(r'"', r'"');
return '"${escaped}"';
}

—
Reply to this email directly or view it on GitHub
https://github.com/angular/angular.dart/pull/944/files#r11912639.

[vicb]

Oops forget what I have said. It should be just fine (latest PR comment).

LGTM once the inline comment is adressed

[vsavkin]

@vicb I've pushed the changes.

Also, I've noticed that the injected Parser is not being used. So I'm thinking of creating a PR removing it. But it is an API change, and I'm not sure if it should go thorough some sort of approval process.

[vicb]

Thanks!

The parser is a leftover from #788, you can safely remove it in this PR.

[vsavkin]

Removed the parse field. Thanks.

[vicb]

closed via 9a45829