fix(compiler): bypass expansion form tokenizing in ngNonBindable blocks #12178

tiagoroldao · 2016-10-09T02:07:10Z

Please check if the PR fulfills these requirements

The commit message follows our guidelines: https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit
Tests for the changes have been added (for bug fixes / features)
[not required] Docs have been added / updated (for bug fixes / features)

What kind of change does this PR introduce? (check one with "x")

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Other... Please describe:

What is the current behavior? (You can also link to an open issue here)
See #11859

Does this PR introduce a breaking change? (check one with "x")

[ ] Yes
[x] No

If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...

Other information:

fixes #11859

vicb · 2016-10-10T06:35:52Z

modules/@angular/compiler/src/ml_parser/lexer.ts

@@ -88,6 +89,8 @@ class _Tokenizer {
  private _currentTokenType: TokenType;
  private _expansionCaseStack: TokenType[] = [];
  private _inInterpolation: boolean = false;
+  private _inNonBindableBlock: boolean = false;


Should this be a getter (depth gt 0)

vicb · 2016-10-10T06:36:41Z

modules/@angular/compiler/src/ml_parser/lexer.ts

@@ -60,6 +60,7 @@ export function tokenize(
 }

 const _CR_OR_CRLF_REGEXP = /\r\n?/g;
+const NG_NON_BINDABLE_ATTR = 'ngNonBindable';


Could this be exported from the template parser ?

tiagoroldao · 2016-10-10T15:06:18Z

Reviewed comments and ammended, @vicb

vicb · 2016-10-10T17:32:35Z

modules/@angular/compiler/src/ml_parser/lexer.ts

    try {
      if (!chars.isAsciiLetter(this._peek)) {
        throw this._createError(_unexpectedCharacterErrorMsg(this._peek), this._getSpan());
      }
      const nameStart = this._index;
      this._consumeTagOpenStart(start);
+      if (this._inNonBindableBlock()) {


merge this test with the code below,

push all AttrName tokens to an array,

if (this._inNonBindableBlock()) { depth++; } else if (attrNames.some(isNonBindableAttribute)) { depth = 1; }

vicb · 2016-10-10T17:33:39Z

modules/@angular/compiler/src/ml_parser/lexer.ts

@@ -640,6 +655,8 @@ class _Tokenizer {
    }
  }

+  private _inNonBindableBlock(): boolean { return this._nonBindableBlockDepth > 0; }


Block -> Element

vicb · 2016-10-10T17:35:01Z

modules/@angular/compiler/src/ml_parser/lexer.ts

@@ -523,6 +535,9 @@ class _Tokenizer {
    this._attemptCharCodeUntilFn(isNotWhitespace);
    this._requireCharCode(chars.$GT);
    this._endToken(prefixAndName);
+    if (this._inNonBindableBlock()) {


some tag are implicitly closed
<ul><li ngNonBindable>blabla<li>{{ 'hello' }}</ul>
Please fix and add a test.

Probably not easy as the logic is in the parser...

May be we should move ICU logic into the parser ??

Yeah, I was just dwelling on that - makes the explicit close-open tags mush more appealing! 😄

It's definitely doable, specially since the lexer does get a _getTagDefinition getter we can use. Moving all code to a checker function that runs after a tag open and a tag close, instead of sprinkling it around the class might do the trick.

A first draft for clarity. Let me know if you prefer to move the logic/approach this in another way.

private _checkNonBindableElementDepth () { let currentToken: Token = this.tokens[this.tokens.length - 1]; if (currentToken.type === TokenType.TAG_OPEN_END || currentToken.type === TokenType.TAG_OPEN_END_VOID || currentToken.type === TokenType.TAG_CLOSE) { // check if this closes any previous, react } if (currentToken.type === TokenType.TAG_OPEN_END) { if (this._inNonBindableElement()) { this._nonBindableElementDepth++; } else if (/* tag contains ngNonBindable attr */){ this._nonBindableElementDepth = 1; } } else if (currentToken.type === TokenType.TAG_CLOSE) { if (this._inNonBindableElement()) { this._nonBindableElementDepth--; } } };

vicb · 2016-10-10T17:37:49Z

Could you please make sure that we have integration test for

ICU like,
interpolation like
in non bindable nodes ?

Thanks !

tiagoroldao · 2016-10-11T01:45:13Z

@vicb changed the approach to keep (and cull) an tag token array, which ended up cleaner (code all in one place). None of the original code was kept, so please re-review as fresh.

vicb · 2016-10-13T00:49:06Z

modules/@angular/compiler/src/ml_parser/lexer.ts

@@ -423,6 +426,7 @@ class _Tokenizer {
    let savedPos = this._savePosition();
    let tagName: string;
    let lowercaseTagName: string;
+    let attributeStart: number;


vicb · 2016-10-13T00:50:19Z

modules/@angular/compiler/src/ml_parser/lexer.ts

+  private _inNonBindableElement(): boolean { return this._nonBindableElementArr.length > 0; }
+
+  private _checkNonBindableElementDepth() {
+    let currentToken: Token = this.tokens[this.tokens.length - 1];


_check... makes me expect a boolean or a throw.

find a better name

add some inline docs // ...

const currentToken

vicb · 2016-10-13T00:51:11Z

modules/@angular/compiler/src/ml_parser/lexer.ts

+  private _checkNonBindableElementDepth() {
+    let currentToken: Token = this.tokens[this.tokens.length - 1];
+
+    if (this._inNonBindableElement() && (currentToken.type === TokenType.TAG_OPEN_END ||


=== || === || === should probably be extracted in a fn

vicb · 2016-10-13T00:52:57Z

modules/@angular/compiler/src/ml_parser/lexer.ts

+        this.tokens, t => t.type === TokenType.TAG_OPEN_START || t.type === TokenType.TAG_CLOSE);
+  }
+
+  private _NonBindableAttrInCurrentElement(): boolean {


vicb · 2016-10-13T00:53:11Z

modules/@angular/compiler/src/ml_parser/lexer.ts

@@ -711,3 +779,15 @@ function mergeTextTokens(srcTokens: Token[]): Token[] {

  return dstTokens;
 }
+
+function findPreviousToken(
+    array: Token[], filter: (token: Token) => boolean,


filter -> predicate ?

vicb · 2016-10-13T00:54:21Z

modules/@angular/compiler/src/ml_parser/lexer.ts

+
+function findPreviousToken(
+    array: Token[], filter: (token: Token) => boolean,
+    stopWhen: (token: Token) => boolean = t => false): Token {


lastPredicate

vicb · 2016-10-13T00:54:31Z

modules/@angular/compiler/src/ml_parser/lexer.ts

+    array: Token[], filter: (token: Token) => boolean,
+    stopWhen: (token: Token) => boolean = t => false): Token {
+  let index = array.length - 1;
+  while (index >= 0 && !stopWhen(array[index])) {


vicb · 2016-10-13T00:55:57Z

Added some comments but I'm not sure if this logic belongs here or in the parser.

Have you tried / thought about detecting ICUs in the parser ?

tiagoroldao · 2016-10-15T09:37:01Z

Fixed the code according to your comments.

Re: refactor:
Don't know if it should - moving tokenisation to the parser seems to further complicate things - wouldn't logic meant Interpolation detection also need to be moved?
Let me know what you think. If I go down that route will need to explore the parser further - may need time and help.

tiagoroldao · 2016-10-17T18:25:26Z

Also - The failed check seems unrelated to this work - is this correct?

tiagoroldao · 2016-10-28T10:25:57Z

@vicb, can you verify this PR? I disagree with the need for moving responsibilities, and assume the test failure was incidental and not related.

fixes #11859

tbosch · 2017-06-06T16:18:16Z

We are closing this PR because it is unclear whether this is the right fix and we need to finalize the design before committing to a solution.

angular-automatic-lock-bot · 2019-09-11T18:21:28Z

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.}

googlebot added the cla: yes label Oct 9, 2016

vicb reviewed Oct 10, 2016

View reviewed changes

tbosch assigned vicb Oct 10, 2016

vicb added the action: review The PR is still awaiting reviews from at least one requested reviewer label Oct 10, 2016

vicb reviewed Oct 10, 2016

View reviewed changes

vicb added the action: cleanup The PR is in need of cleanup, either due to needing a rebase or in response to comments from reviews label Oct 10, 2016

pkozlowski-opensource mentioned this pull request Oct 12, 2016

Can't include static json content as part of template html #10886

Closed

vicb reviewed Oct 13, 2016

View reviewed changes

pkozlowski-opensource added the area: core Issues related to the framework runtime label Oct 18, 2016

fix(compiler): bypass expansion form tokenizing in ngNonBindable blocks

f27bce0

fixes #11859

chuckjaz added the type: bug/fix label May 25, 2017

tbosch closed this Jun 6, 2017

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(compiler): bypass expansion form tokenizing in ngNonBindable blocks #12178

fix(compiler): bypass expansion form tokenizing in ngNonBindable blocks #12178

tiagoroldao commented Oct 9, 2016

vicb Oct 10, 2016

vicb Oct 10, 2016

tiagoroldao commented Oct 10, 2016

vicb Oct 10, 2016

vicb Oct 10, 2016

vicb Oct 10, 2016

vicb Oct 10, 2016

tiagoroldao Oct 10, 2016

tiagoroldao Oct 10, 2016 •

edited

Loading

vicb commented Oct 10, 2016

tiagoroldao commented Oct 11, 2016

vicb Oct 13, 2016

vicb Oct 13, 2016

vicb Oct 13, 2016

vicb Oct 13, 2016

vicb Oct 13, 2016

vicb Oct 13, 2016

vicb Oct 13, 2016

vicb Oct 13, 2016

vicb commented Oct 13, 2016

tiagoroldao commented Oct 15, 2016 •

edited

Loading

tiagoroldao commented Oct 17, 2016

tiagoroldao commented Oct 28, 2016

tbosch commented Jun 6, 2017

angular-automatic-lock-bot bot commented Sep 11, 2019

fix(compiler): bypass expansion form tokenizing in ngNonBindable blocks #12178

fix(compiler): bypass expansion form tokenizing in ngNonBindable blocks #12178

Conversation

tiagoroldao commented Oct 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagoroldao commented Oct 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tiagoroldao Oct 10, 2016 • edited Loading

Choose a reason for hiding this comment

vicb commented Oct 10, 2016

tiagoroldao commented Oct 11, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Oct 13, 2016

tiagoroldao commented Oct 15, 2016 • edited Loading

tiagoroldao commented Oct 17, 2016

tiagoroldao commented Oct 28, 2016

tbosch commented Jun 6, 2017

angular-automatic-lock-bot bot commented Sep 11, 2019

tiagoroldao Oct 10, 2016 •

edited

Loading

tiagoroldao commented Oct 15, 2016 •

edited

Loading