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(compiler): handle strings inside bindings that contain binding characters #39826

Closed
wants to merge 4 commits into from

Conversation

crisbeto
Copy link
Member

@crisbeto crisbeto commented Nov 24, 2020

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.

@google-cla google-cla bot added the cla: yes label Nov 24, 2020
@crisbeto crisbeto force-pushed the 39601/interpolation-quoted branch 2 times, most recently from 1e58121 to 13f72e3 Compare Nov 24, 2020
@crisbeto crisbeto marked this pull request as ready for review Nov 24, 2020
@crisbeto crisbeto added action: review comp: compiler compiler: parser target: patch This PR is targeted for the next patch release labels Nov 24, 2020
@ngbot ngbot bot modified the milestone: needsTriage Nov 24, 2020
@pullapprove pullapprove bot requested a review from AndrewKushnir Nov 24, 2020
Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

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.

Copy link
Member

@petebacondarwin petebacondarwin left a comment

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 Outdated Show resolved Hide resolved
packages/compiler/src/expression_parser/parser.ts Outdated Show resolved Hide resolved
@crisbeto crisbeto force-pushed the 39601/interpolation-quoted branch from 13f72e3 to 1512e3b Compare Nov 25, 2020
@crisbeto
Copy link
Member Author

@crisbeto crisbeto commented Nov 25, 2020

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

@crisbeto crisbeto force-pushed the 39601/interpolation-quoted branch from 96801fb to 674d300 Compare Nov 28, 2020
@crisbeto crisbeto added action: merge PR author is ready for this to merge action: merge-assistance CARETAKER: please help get this green action: presubmit A standard presubmit is running / required and removed action: review labels Nov 30, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Nov 30, 2020

Presubmit.

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 1, 2020

@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
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 1, 2020

Global presubmit.

@jessicajaniuk jessicajaniuk removed the action: merge PR author is ready for this to merge label Dec 1, 2020
@crisbeto crisbeto force-pushed the 39601/interpolation-quoted branch from 83c70bd to 1a96c5e Compare Dec 8, 2020
@crisbeto
Copy link
Member Author

@crisbeto crisbeto commented Dec 8, 2020

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?

Copy link
Contributor

@AndrewKushnir AndrewKushnir left a comment

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?

@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 9, 2020

@ngbot ngbot bot removed this from the needsTriage milestone Dec 9, 2020
@AndrewKushnir AndrewKushnir removed action: presubmit A standard presubmit is running / required state: blocked labels Dec 9, 2020
@AndrewKushnir
Copy link
Contributor

@AndrewKushnir AndrewKushnir commented Dec 9, 2020

FYI, presubmits went well 👍

JoostK
JoostK approved these changes Dec 9, 2020
Copy link
Member

@JoostK JoostK left a comment

LGTM!

@crisbeto crisbeto added the action: merge PR author is ready for this to merge label Dec 9, 2020
@AndrewKushnir AndrewKushnir removed the action: merge-assistance CARETAKER: please help get this green label Dec 9, 2020
alxhub pushed a commit that referenced this issue Dec 10, 2020
…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
@alxhub alxhub closed this in dc6d40e Dec 10, 2020
zarend pushed a commit to zarend/angular that referenced this issue Dec 11, 2020
…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
it('should parse interpolation with escaped backslashes', () => {
checkInterpolation(`{{foo.split('\\\\')}}`, `{{ foo.split("\\") }}`);
checkInterpolation(`{{foo.split('\\\\\\\\')}}`, `{{ foo.split("\\\\") }}`);
checkInterpolation(`{{foo.split('\\\\\\\\\\\\')}}`, `{{ foo.split("\\\\\\") }}`);
Copy link
Contributor

@lazarljubenovic lazarljubenovic Dec 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@ngbot ngbot bot modified the milestone: Backlog Dec 16, 2020
crisbeto added a commit to crisbeto/angular that referenced this issue Dec 26, 2020
…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.
twerske pushed a commit to twerske/angular that referenced this issue Jan 5, 2021
…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
josephperrott pushed a commit that referenced this issue Jan 5, 2021
…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
josephperrott pushed a commit that referenced this issue Jan 5, 2021
…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
Copy link

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

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.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Jan 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge PR author is ready for this to merge cla: yes comp: compiler comp: core Runtime issues compiler: parser risk: medium target: patch This PR is targeted for the next patch release type: bug/fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants