fix(compiler): handle strings inside bindings that contain binding characters #39826

crisbeto · 2020-11-24T17:08:53Z

Currently the compiler treats something like {{ '{{a}}' }} as a nested binding and throws an error, because it doesn't account for quotes when it looks for binding characters. These changes add a bit of logic to skip over text inside quotes when parsing.

AndrewKushnir

The change looks good, I've just left a few comments.

I think it'd be great if @petebacondarwin can have a look as well, since he did some work in expression parser recently.

Thank you.

packages/compiler/src/expression_parser/parser.ts

packages/compiler/test/template_parser/template_parser_spec.ts

packages/compiler/src/expression_parser/parser.ts

petebacondarwin

Nice little fix @crisbeto - I agree with @AndrewKushnir's test addition suggestion. I made some suggestions of my own for the tests and the algorithm. Feel free to consider them :-)

packages/compiler/src/expression_parser/parser.ts

packages/compiler/test/template_parser/template_parser_spec.ts

crisbeto · 2020-11-25T18:59:55Z

Thank you for the reviews everybody, all of the should be addressed now.

packages/compiler/test/expression_parser/parser_spec.ts

AndrewKushnir · 2020-11-30T16:42:53Z

Presubmit.

AndrewKushnir · 2020-12-01T03:23:24Z

@crisbeto just a quick update:

it looks like there is a merge conflict now, could you please rebase when you get a chance?
presubmit indicated a problem with one of the targets (thus adding the "blocked" label for now). The problem seems to be related to a typo in a template, but I need a bit more time to investigate it and probably do a global run to make sure there is no regression in other places.

Will keep this thread updated.

AndrewKushnir · 2020-12-01T03:28:42Z

Global presubmit.

…ding characters

crisbeto · 2020-12-08T19:55:30Z

Thank you for looking into it @AndrewKushnir. I've added logic to handle the two use cases that you mentioned and included some more unit tests. Can we run it through again?

AndrewKushnir

I've added logic to handle the two use cases that you mentioned and included some more unit tests. Can we run it through again?

Thanks @crisbeto! I've just added a quick comment on the // case to see if we can add more tests (and whether the way we handle that is correct). Could you please have a look when you get a chance?

packages/compiler/test/expression_parser/parser_spec.ts

AndrewKushnir · 2020-12-09T02:20:34Z

Presubmit #2 + Global Presubmit #2.

AndrewKushnir · 2020-12-09T19:13:15Z

FYI, presubmits went well 👍

JoostK

LGTM!

…aracters (#39826) Currently the compiler treats something like `{{ '{{a}}' }}` as a nested binding and throws an error, because it doesn't account for quotes when it looks for binding characters. These changes add a bit of logic to skip over text inside quotes when parsing. Fixes #39601. PR Close #39826

…aracters (angular#39826) Currently the compiler treats something like `{{ '{{a}}' }}` as a nested binding and throws an error, because it doesn't account for quotes when it looks for binding characters. These changes add a bit of logic to skip over text inside quotes when parsing. Fixes angular#39601. PR Close angular#39826

lazarljubenovic · 2020-12-16T10:18:05Z

packages/compiler/test/expression_parser/parser_spec.ts

+    it('should parse interpolation with escaped backslashes', () => {
+      checkInterpolation(`{{foo.split('\\\\')}}`, `{{ foo.split("\\") }}`);
+      checkInterpolation(`{{foo.split('\\\\\\\\')}}`, `{{ foo.split("\\\\") }}`);
+      checkInterpolation(`{{foo.split('\\\\\\\\\\\\')}}`, `{{ foo.split("\\\\\\") }}`);


A tip: this code would greatly benefit from String.raw`\no \escaping \here`. See MDN for details.

…ng in property binding Currently we check whether a property binding contains an interpolation using a regex so that we can throw an error. The problem is that the regex doesn't account for quotes which means that something like `[prop]="'{{ foo }}'"` will be considered an error, even though it's not actually an interpolation. These changes build on top of the logic from angular#39826 to account for interpolation characters inside quotes. Fixes angular#39601.

…ng in property binding (#40267) Currently we check whether a property binding contains an interpolation using a regex so that we can throw an error. The problem is that the regex doesn't account for quotes which means that something like `[prop]="'{{ foo }}'"` will be considered an error, even though it's not actually an interpolation. These changes build on top of the logic from #39826 to account for interpolation characters inside quotes. Fixes #39601. PR Close #40267

angular-automatic-lock-bot · 2021-01-16T17:08:13Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

google-cla bot added the cla: yes label Nov 24, 2020

crisbeto force-pushed the 39601/interpolation-quoted branch 2 times, most recently from 1e58121 to 13f72e3 Compare November 24, 2020 17:51

crisbeto marked this pull request as ready for review November 24, 2020 18:30

crisbeto added action: review The PR is still awaiting reviews from at least one requested reviewer area: compiler Issues related to `ngc`, Angular's template compiler compiler: parser target: patch This PR is targeted for the next patch release labels Nov 24, 2020

ngbot bot modified the milestone: needsTriage Nov 24, 2020

crisbeto added the type: bug/fix label Nov 24, 2020

pullapprove bot requested a review from AndrewKushnir November 24, 2020 18:30

AndrewKushnir reviewed Nov 25, 2020

View reviewed changes

packages/compiler/src/expression_parser/parser.ts Outdated Show resolved Hide resolved

packages/compiler/test/template_parser/template_parser_spec.ts Show resolved Hide resolved

packages/compiler/src/expression_parser/parser.ts Outdated Show resolved Hide resolved

AndrewKushnir requested a review from petebacondarwin November 25, 2020 01:25

petebacondarwin approved these changes Nov 25, 2020

View reviewed changes

packages/compiler/src/expression_parser/parser.ts Outdated Show resolved Hide resolved

packages/compiler/src/expression_parser/parser.ts Outdated Show resolved Hide resolved

packages/compiler/test/template_parser/template_parser_spec.ts Outdated Show resolved Hide resolved

crisbeto force-pushed the 39601/interpolation-quoted branch from 13f72e3 to 1512e3b Compare November 25, 2020 17:05

JoostK reviewed Nov 25, 2020

View reviewed changes

packages/compiler/test/template_parser/template_parser_spec.ts Show resolved Hide resolved

JoostK reviewed Nov 25, 2020

View reviewed changes

packages/compiler/test/expression_parser/parser_spec.ts Show resolved Hide resolved

crisbeto force-pushed the 39601/interpolation-quoted branch from 96801fb to 674d300 Compare November 28, 2020 10:17

AndrewKushnir added the risk: medium label Nov 30, 2020

AndrewKushnir added the state: blocked label Dec 1, 2020

jessicajaniuk removed the action: merge The PR is ready for merge by the caretaker label Dec 1, 2020

crisbeto added 2 commits December 8, 2020 17:34

fixup! fix(compiler): handle strings inside bindings that contain bin…

f56464e

…ding characters

fixup! fix(compiler): handle strings inside bindings that contain bin…

1a96c5e

…ding characters

crisbeto force-pushed the 39601/interpolation-quoted branch from 83c70bd to 1a96c5e Compare December 8, 2020 19:25

AndrewKushnir reviewed Dec 8, 2020

View reviewed changes

packages/compiler/test/expression_parser/parser_spec.ts Show resolved Hide resolved

pullapprove bot removed the area: compiler Issues related to `ngc`, Angular's template compiler label Dec 9, 2020

ngbot bot removed this from the needsTriage milestone Dec 9, 2020

AndrewKushnir removed action: presubmit The PR is in need of a google3 presubmit state: blocked labels Dec 9, 2020

AndrewKushnir approved these changes Dec 9, 2020

View reviewed changes

JoostK approved these changes Dec 9, 2020

View reviewed changes

crisbeto added the action: merge The PR is ready for merge by the caretaker label Dec 9, 2020

AndrewKushnir removed the merge: caretaker note Alert the caretaker performing the merge to check the PR for an out of normal action needed or note label Dec 9, 2020

alxhub closed this in dc6d40e Dec 10, 2020

JoostK mentioned this pull request Dec 15, 2020

Using {{ and }} in strings in templates breaks the compiler #40133

Closed

lazarljubenovic reviewed Dec 16, 2020

View reviewed changes

pullapprove bot added area: compiler Issues related to `ngc`, Angular's template compiler area: core Issues related to the framework runtime labels Dec 16, 2020

ngbot bot modified the milestone: Backlog Dec 16, 2020

crisbeto mentioned this pull request Dec 26, 2020

fix(compiler): don't report parse error for interpolation inside string in property binding #40267

Closed

angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler): handle strings inside bindings that contain binding characters #39826

fix(compiler): handle strings inside bindings that contain binding characters #39826

crisbeto commented Nov 24, 2020 •

edited

Loading

AndrewKushnir left a comment •

edited

Loading

petebacondarwin left a comment

crisbeto commented Nov 25, 2020

AndrewKushnir commented Nov 30, 2020

AndrewKushnir commented Dec 1, 2020 •

edited

Loading

AndrewKushnir commented Dec 1, 2020

crisbeto commented Dec 8, 2020

AndrewKushnir left a comment

AndrewKushnir commented Dec 9, 2020 •

edited

Loading

AndrewKushnir commented Dec 9, 2020

JoostK left a comment

lazarljubenovic Dec 16, 2020

angular-automatic-lock-bot bot commented Jan 16, 2021

fix(compiler): handle strings inside bindings that contain binding characters #39826

fix(compiler): handle strings inside bindings that contain binding characters #39826

Conversation

crisbeto commented Nov 24, 2020 • edited Loading

AndrewKushnir left a comment • edited Loading

Choose a reason for hiding this comment

petebacondarwin left a comment

Choose a reason for hiding this comment

crisbeto commented Nov 25, 2020

AndrewKushnir commented Nov 30, 2020

AndrewKushnir commented Dec 1, 2020 • edited Loading

AndrewKushnir commented Dec 1, 2020

crisbeto commented Dec 8, 2020

AndrewKushnir left a comment

Choose a reason for hiding this comment

AndrewKushnir commented Dec 9, 2020 • edited Loading

AndrewKushnir commented Dec 9, 2020

JoostK left a comment

Choose a reason for hiding this comment

lazarljubenovic Dec 16, 2020

Choose a reason for hiding this comment

angular-automatic-lock-bot bot commented Jan 16, 2021

crisbeto commented Nov 24, 2020 •

edited

Loading

AndrewKushnir left a comment •

edited

Loading

AndrewKushnir commented Dec 1, 2020 •

edited

Loading

AndrewKushnir commented Dec 9, 2020 •

edited

Loading