GeezerVison

Copy link

Copy Markdown

Contributor

artaommahe and tonivj5 reacted with heart emoji

dylhunn force-pushed the template-ingestion branch 17 times, most recently from 383f4c6 to 81c447f Compare December 9, 2023 23:27

dylhunn marked this pull request as ready for review December 9, 2023 23:28

dylhunn requested review from alxhub and mmalerba December 9, 2023 23:28

dylhunn force-pushed the template-ingestion branch from 81c447f to cafbf59 Compare December 9, 2023 23:35

…3457)

Consider the case:

```
<button *ngIf="true" [@anim]="field"></button>
```

Only the inner `button` should recieve a `property` instruction for the animation binding. We were previously emitting one for the implicit `ng-template` as well, and collecting it into the consts for the `ng-template`. Both of these issues are now fixed.

PR Close #53457

…53457)

Responding to comments on #53457 about the previous sequence of template binding PRs.

PR Close #53457

… template views (#53457)

The Template Pipeline has had a number of tricky bugs involving bindings on structural elements.

Consider this template:

```
<div *ngIf="true" [class.bar]="field"></div>
```

We were incorrectly emitting `ɵɵclassProp` on *both* the template's view, and the inner view. The solution is to just emit an extracted attribute on the enclosing template, so it still shows up in the const array, but does not affect the update block.

We will refactor binding ingestion soon, but this commit improves our correctness before any big refactor.

PR Close #53457

Currently Template Pipeline's ingest phase is very complex, especially when it comes to ingesting bindings.

In this commit, we make some superficial simplifications, in preparation for a larger refactoring. For example, we pull out common code such as `convertAstWithInterpolation` and the `i18n.Message` checks. This enormously shrinks the main binding ingestion functions.

In addition, we reorder the binding kind and flags code above `ingestBindings`, so that `ingestBindings` and `ingestBinding` can be viewed together.

PR Close #53457

…ates (#53457)

Previously, we had `ingestBindings` and `ingestBinding`, which required tons of cases to support both elements and templates.

Now, we have two separate functions, `ingestElementBindings` and `ingestTemplateBindings`.

Thanks to the previous refactoring work, `ingestBinding` is now extremely compact. In fact, it's so compact that, in the elements case, it can just be inlined! Therefore, element binding ingestion is now quite easy to read.

The template case continues to be pretty gnarly, although I have already removed some code. In subsequent commits, we will simplify it even further.

PR Close #53457

…f `createTemplateBinding` (#53457)

It turns out that `BindingFlags.BindingTargetsTemplate` is actally a redundant property! It will be true in either of the following cases:
1. The template is a normal non-structural `ng-template`. We already know this from `TemplateKind`.
2. The binding came from `templateAttrs` (instead of `attrs`). We have this information in `BindingFlags.IsStructuralTemplateAttribute`.

Therefore, I can just eliminate `BindingFlags.BindingTargetsTemplate`. There's no reason to keep `BindingFlags` around for a single value, so I convert `BindingFlags.IsStructuralTemplateAttribute` to a boolean parameter (with the eventual goal of eliminating it entirely).

Additionally, because element binding ingestion now calls `ir.createBindingOp` inline, it was difficult to compare it to template binding ingestion, which uses the  `createTemplateBinding` helper. I have changed the parameter order of `createTemplateBinding` to closely mimic `ir.createBindingOp`. This will both make the code easier to read, and allow me to easily replace one with the other in the future.

Lastly: the template binding ingestion function is the site of much of the binding ingestion complexity. Add an explanatory function comment.

PR Close #53457

…emplate` elements (#53457)

The behavior of explicit bindings on `ng-template`s was untested, and we differed from `TemplateDefinitionBuilder` significantly. We now have much more similar behavior, although not 100% identical.

For example, consider this templarte:
```
<ng-template l="l1" [p]="p1" [attr.a]="a1" [class.c]="c1"></ng-template>
```

It's not clear what a class binding on an `ng-template` would actually do. Nonetheless, it's well-defined behavior in TemplateDefinitionBuilder, which emits `property` instructions for all three bindings, and people actually do this in google3.

Note that some of these bindings don't really make much sense, but we have to support them for compatibility purposes.

See comments for an in-depth explanation of all the logic.

Also, add a test to exercise the problematic case.

PR Close #53457

…3457)

Consider the case:

```
<button *ngIf="true" [@anim]="field"></button>
```

Only the inner `button` should recieve a `property` instruction for the animation binding. We were previously emitting one for the implicit `ng-template` as well, and collecting it into the consts for the `ng-template`. Both of these issues are now fixed.

PR Close #53457

…53457)

Responding to comments on #53457 about the previous sequence of template binding PRs.

PR Close #53457

… template views (angular#53457)

The Template Pipeline has had a number of tricky bugs involving bindings on structural elements.

Consider this template:

```
<div *ngIf="true" [class.bar]="field"></div>
```

We were incorrectly emitting `ɵɵclassProp` on *both* the template's view, and the inner view. The solution is to just emit an extracted attribute on the enclosing template, so it still shows up in the const array, but does not affect the update block.

We will refactor binding ingestion soon, but this commit improves our correctness before any big refactor.

PR Close angular#53457

Currently Template Pipeline's ingest phase is very complex, especially when it comes to ingesting bindings.

In this commit, we make some superficial simplifications, in preparation for a larger refactoring. For example, we pull out common code such as `convertAstWithInterpolation` and the `i18n.Message` checks. This enormously shrinks the main binding ingestion functions.

In addition, we reorder the binding kind and flags code above `ingestBindings`, so that `ingestBindings` and `ingestBinding` can be viewed together.

PR Close angular#53457

…ates (angular#53457)

Previously, we had `ingestBindings` and `ingestBinding`, which required tons of cases to support both elements and templates.

Now, we have two separate functions, `ingestElementBindings` and `ingestTemplateBindings`.

Thanks to the previous refactoring work, `ingestBinding` is now extremely compact. In fact, it's so compact that, in the elements case, it can just be inlined! Therefore, element binding ingestion is now quite easy to read.

The template case continues to be pretty gnarly, although I have already removed some code. In subsequent commits, we will simplify it even further.

PR Close angular#53457

…f `createTemplateBinding` (angular#53457)

It turns out that `BindingFlags.BindingTargetsTemplate` is actally a redundant property! It will be true in either of the following cases:
1. The template is a normal non-structural `ng-template`. We already know this from `TemplateKind`.
2. The binding came from `templateAttrs` (instead of `attrs`). We have this information in `BindingFlags.IsStructuralTemplateAttribute`.

Therefore, I can just eliminate `BindingFlags.BindingTargetsTemplate`. There's no reason to keep `BindingFlags` around for a single value, so I convert `BindingFlags.IsStructuralTemplateAttribute` to a boolean parameter (with the eventual goal of eliminating it entirely).

Additionally, because element binding ingestion now calls `ir.createBindingOp` inline, it was difficult to compare it to template binding ingestion, which uses the  `createTemplateBinding` helper. I have changed the parameter order of `createTemplateBinding` to closely mimic `ir.createBindingOp`. This will both make the code easier to read, and allow me to easily replace one with the other in the future.

Lastly: the template binding ingestion function is the site of much of the binding ingestion complexity. Add an explanatory function comment.

PR Close angular#53457

…emplate` elements (angular#53457)

The behavior of explicit bindings on `ng-template`s was untested, and we differed from `TemplateDefinitionBuilder` significantly. We now have much more similar behavior, although not 100% identical.

For example, consider this templarte:
```
<ng-template l="l1" [p]="p1" [attr.a]="a1" [class.c]="c1"></ng-template>
```

It's not clear what a class binding on an `ng-template` would actually do. Nonetheless, it's well-defined behavior in TemplateDefinitionBuilder, which emits `property` instructions for all three bindings, and people actually do this in google3.

Note that some of these bindings don't really make much sense, but we have to support them for compatibility purposes.

See comments for an in-depth explanation of all the logic.

Also, add a test to exercise the problematic case.

PR Close angular#53457

…gular#53457)

Consider the case:

```
<button *ngIf="true" [@anim]="field"></button>
```

Only the inner `button` should recieve a `property` instruction for the animation binding. We were previously emitting one for the implicit `ng-template` as well, and collecting it into the consts for the `ng-template`. Both of these issues are now fixed.

PR Close angular#53457

…ngular#53457)

Responding to comments on angular#53457 about the previous sequence of template binding PRs.

PR Close angular#53457

… template views (angular#53457)

The Template Pipeline has had a number of tricky bugs involving bindings on structural elements.

Consider this template:

```
<div *ngIf="true" [class.bar]="field"></div>
```

We were incorrectly emitting `ɵɵclassProp` on *both* the template's view, and the inner view. The solution is to just emit an extracted attribute on the enclosing template, so it still shows up in the const array, but does not affect the update block.

We will refactor binding ingestion soon, but this commit improves our correctness before any big refactor.

PR Close angular#53457

Currently Template Pipeline's ingest phase is very complex, especially when it comes to ingesting bindings.

In this commit, we make some superficial simplifications, in preparation for a larger refactoring. For example, we pull out common code such as `convertAstWithInterpolation` and the `i18n.Message` checks. This enormously shrinks the main binding ingestion functions.

In addition, we reorder the binding kind and flags code above `ingestBindings`, so that `ingestBindings` and `ingestBinding` can be viewed together.

PR Close angular#53457

…ates (angular#53457)

Previously, we had `ingestBindings` and `ingestBinding`, which required tons of cases to support both elements and templates.

Now, we have two separate functions, `ingestElementBindings` and `ingestTemplateBindings`.

Thanks to the previous refactoring work, `ingestBinding` is now extremely compact. In fact, it's so compact that, in the elements case, it can just be inlined! Therefore, element binding ingestion is now quite easy to read.

The template case continues to be pretty gnarly, although I have already removed some code. In subsequent commits, we will simplify it even further.

PR Close angular#53457

…f `createTemplateBinding` (angular#53457)

It turns out that `BindingFlags.BindingTargetsTemplate` is actally a redundant property! It will be true in either of the following cases:
1. The template is a normal non-structural `ng-template`. We already know this from `TemplateKind`.
2. The binding came from `templateAttrs` (instead of `attrs`). We have this information in `BindingFlags.IsStructuralTemplateAttribute`.

Therefore, I can just eliminate `BindingFlags.BindingTargetsTemplate`. There's no reason to keep `BindingFlags` around for a single value, so I convert `BindingFlags.IsStructuralTemplateAttribute` to a boolean parameter (with the eventual goal of eliminating it entirely).

Additionally, because element binding ingestion now calls `ir.createBindingOp` inline, it was difficult to compare it to template binding ingestion, which uses the  `createTemplateBinding` helper. I have changed the parameter order of `createTemplateBinding` to closely mimic `ir.createBindingOp`. This will both make the code easier to read, and allow me to easily replace one with the other in the future.

Lastly: the template binding ingestion function is the site of much of the binding ingestion complexity. Add an explanatory function comment.

PR Close angular#53457

…emplate` elements (angular#53457)

The behavior of explicit bindings on `ng-template`s was untested, and we differed from `TemplateDefinitionBuilder` significantly. We now have much more similar behavior, although not 100% identical.

For example, consider this templarte:
```
<ng-template l="l1" [p]="p1" [attr.a]="a1" [class.c]="c1"></ng-template>
```

It's not clear what a class binding on an `ng-template` would actually do. Nonetheless, it's well-defined behavior in TemplateDefinitionBuilder, which emits `property` instructions for all three bindings, and people actually do this in google3.

Note that some of these bindings don't really make much sense, but we have to support them for compatibility purposes.

See comments for an in-depth explanation of all the logic.

Also, add a test to exercise the problematic case.

PR Close angular#53457

…gular#53457)

Consider the case:

```
<button *ngIf="true" [@anim]="field"></button>
```

Only the inner `button` should recieve a `property` instruction for the animation binding. We were previously emitting one for the implicit `ng-template` as well, and collecting it into the consts for the `ng-template`. Both of these issues are now fixed.

PR Close angular#53457

…ngular#53457)

Responding to comments on angular#53457 about the previous sequence of template binding PRs.

PR Close angular#53457