Skip to content

fix(ivy): tokenize escaped new lines in inline templates - #28844

Closed
cexbrayat wants to merge 1 commit into
angular:masterfrom
cexbrayat:fix/lexer
Closed

cexbrayat wants to merge 1 commit into
angular:masterfrom
cexbrayat:fix/lexer

Conversation

@cexbrayat

Copy link
Copy Markdown
Member

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • angular.io application / infrastructure changes
  • Other... Please describe:

What is the current behavior?

Issue Number: #28843

What is the new behavior?

The PR introduces two new tests to reproduce the issue found, and a fix/workaround.
I'm not sure this is the ideal way to fix the issue, @petebacondarwin will know better.
But I included the workaround I found to give a little more context about the issue.

Does this PR introduce a breaking change?

  • Yes
  • No

@cexbrayat
cexbrayat requested a review from a team February 19, 2019 22:07
@gkalpak gkalpak added type: bug/fix target: major This PR is targeted for the next major release comp: ivy labels Feb 20, 2019
@ngbot ngbot Bot added this to the needsTriage milestone Feb 20, 2019
@gkalpak gkalpak added the regression Indicates than the issue relates to something that worked in a previous version label Feb 20, 2019

@petebacondarwin petebacondarwin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I love the tests. Nice work @cexbrayat.

I worry that this fix is more of a hack to work around something more serious wrong in the lexer.
I think that instead there should be a fix in the _attemptCharCodeUntilFn or _advance methods...

@cexbrayat

Copy link
Copy Markdown
Member Author

@petebacondarwin Yes, that's what I think as well, but I'm not sure what should be the proper way to fix it... _processEscapeSequence is consuming the first \ (here https://github.com/angular/angular/blob/master/packages/compiler/src/ml_parser/lexer.ts#L537), so this._attemptCharCodeUntilFn(isNameEnd) does not stop until the $LF just after...

@petebacondarwin

Copy link
Copy Markdown
Contributor

Yes. The index should stay at the start of the character. We need to change the _processEscapeSequence method so that we only update that index when we move to the next char.

Also the fix you have would not help for longer escape sequences like variable Unicode ones.

Prior to this fix, inline templates including sequences like `<t\\n>` or `<t a=\\nb>` were failing.
This introduces a dedicated index to track the advance during escape sequences.
The actual index then catches up when the escape sequence is over.
This allows to not consume `$BACKSLASH` tokens when not needed.

Fixes angular#28843
@cexbrayat

Copy link
Copy Markdown
Member Author

@petebacondarwin I think I understand what you mean and attempted a proper fix. The amended commit now has a dedicated index to track the advanced during escape sequences, and the index only catches up when needed.

@cexbrayat

Copy link
Copy Markdown
Member Author

Superseeded by #28978

@cexbrayat cexbrayat closed this Feb 26, 2019
@angular-automatic-lock-bot

Copy link
Copy Markdown

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 Sep 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cla: yes regression Indicates than the issue relates to something that worked in a previous version target: major This PR is targeted for the next major release type: bug/fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants