refactor(compiler): Don't emit properties on structural ng-templates (#51950)

Consider an `ng-template` which is generated as a result of a structural directive:

```
<div *ngFor="let inner of items"
             (click)="onClick(inner)"
             [title]="getTitle()"
             >
```

This should logically expand into something like the following:

```
<ng-template [ngForOf]="..." >
        <div (click)="..." [title]="..."></div>
</ng-template>
```

Note that the `(click)` handler and the `[title]` property are only present on the inner div, *not* on the enclosing generated `ng-template`.

Previously, Template Pipeline would place these bindings on *both* the tempate and the inner element.

However, we can't just remove them completely, because these bindings should still be matchable on the generated `ng-template` (which is very surprising, but nonetheless true).

We resolve this issue with two improvements:
(1) The ingestion step is now much smarter about determining not only if a binding is on a template element, but whether it actually targets that template element.
(2) We use `ExtractedAttributeOp` directly, rather than going through `BindingOp`, to cause the `ng-template` to still receive these bindings in its `consts` array for matching purposes.

PR Close #51950
This commit is contained in:
Dylan Hunn 2023-09-29 16:44:39 -07:00 committed by Alex Rickabaugh
parent 04436cfd60
commit 4b4dd2bf3a
5 changed files with 126 additions and 48 deletions

View file

@ -11,13 +11,13 @@
"files": [
{
"expected": "nested_template_context_template.js",
"templatePipelineExpected": "nested_template_context.pipeline.js",
"generated": "nested_template_context.js"
}
],
"failureMessage": "Incorrect template"
}
],
"skipForTemplatePipeline": true
]
},
{
"description": "should correctly bind to context in nested template with many bindings",

View file

@ -0,0 +1,66 @@
function MyComponent_ul_0_li_1_div_1_Template(rf, ctx) {
if (rf & 1) {
const $s$ = $i0$.ɵɵgetCurrentView();
$i0$.ɵɵelementStart(0, "div", 2);
$i0$.ɵɵlistener("click", function MyComponent_ul_0_li_1_div_1_Template_div_click_0_listener(){
const $inner$ = $i0$.ɵɵrestoreView($s$).$implicit;
const $middle$ = $i0$.ɵɵnextContext().$implicit;
const $outer$ = $i0$.ɵɵnextContext().$implicit;
const $myComp$ = $i0$.ɵɵnextContext();
return $i0$.ɵɵresetView($myComp$.onClick($outer$, $middle$, $inner$));
});
$i0$.ɵɵtext(1);
$i0$.ɵɵelementEnd();
}
if (rf & 2) {
const $inner1$ = ctx.$implicit;
const $middle1$ = $i0$.ɵɵnextContext().$implicit;
const $outer1$ = $i0$.ɵɵnextContext().$implicit;
const $myComp1$ = $i0$.ɵɵnextContext();
$i0$.ɵɵproperty("title", $myComp1$.format($outer1$, $middle1$, $inner1$, $myComp1$.component));
$r3$.ɵɵadvance(1);
$i0$.ɵɵtextInterpolate1(" ", $myComp1$.format($outer1$, $middle1$, $inner1$, $myComp1$.component), " ");
}
}
function MyComponent_ul_0_li_1_Template(rf, ctx) {
if (rf & 1) {
$i0$.ɵɵelementStart(0, "li");
$i0$.ɵɵtemplate(1, MyComponent_ul_0_li_1_div_1_Template, 2, 2, "div", 1);
$i0$.ɵɵelementEnd();
}
if (rf & 2) {
const $myComp2$ = $i0$.ɵɵnextContext(2);
$r3$.ɵɵadvance(1);
$i0$.ɵɵproperty("ngForOf", $myComp2$.items);
}
}
function MyComponent_ul_0_Template(rf, ctx) {
if (rf & 1) {
$i0$.ɵɵelementStart(0, "ul");
$i0$.ɵɵtemplate(1, MyComponent_ul_0_li_1_Template, 2, 1, "li", 0);
$i0$.ɵɵelementEnd();
}
if (rf & 2) {
const $outer2$ = ctx.$implicit;
$r3$.ɵɵadvance(1);
$i0$.ɵɵproperty("ngForOf", $outer2$.items);
}
}
consts: [
[__AttributeMarker.Template__, "ngFor", "ngForOf"],
[__AttributeMarker.Bindings__, "title", "click", __AttributeMarker.Template__, "ngFor", "ngForOf"],
[__AttributeMarker.Bindings__, "click", "title"]
],
template:function MyComponent_Template(rf, ctx){
if (rf & 1) {
$i0$.ɵɵtemplate(0, MyComponent_ul_0_Template, 2, 1, "ul", 0);
}
if (rf & 2) {
$i0$.ɵɵproperty("ngForOf", ctx.items);
}
}

View file

@ -35,7 +35,6 @@ import {phaseNamespace} from './phases/namespace';
import {phaseNaming} from './phases/naming';
import {phaseMergeNextContext} from './phases/next_context_merging';
import {phaseNgContainer} from './phases/ng_container';
import {phaseNoListenersOnTemplates} from './phases/no_listeners_on_templates';
import {phaseNonbindable} from './phases/nonbindable';
import {phaseNullishCoalescing} from './phases/nullish_coalescing';
import {phaseOrdering} from './phases/ordering';
@ -80,7 +79,6 @@ const phases: Phase[] = [
{kind: Kind.Both, fn: phaseParseExtractedStyles},
{kind: Kind.Tmpl, fn: phaseRemoveEmptyBindings},
{kind: Kind.Tmpl, fn: phaseConditionals},
{kind: Kind.Tmpl, fn: phaseNoListenersOnTemplates},
{kind: Kind.Tmpl, fn: phasePipeCreation},
{kind: Kind.Tmpl, fn: phaseI18nTextExtraction},
{kind: Kind.Tmpl, fn: phaseApplyI18nExpressions},

View file

@ -219,7 +219,7 @@ function ingestContent(unit: ViewCompilationUnit, content: t.Content): void {
for (const attr of content.attributes) {
ingestBinding(
unit, op.xref, attr.name, o.literal(attr.value), e.BindingType.Attribute, null,
SecurityContext.NONE, attr.sourceSpan, true, false);
SecurityContext.NONE, attr.sourceSpan, BindingFlags.TextValue);
}
unit.create.push(op);
}
@ -485,16 +485,27 @@ function convertAst(
*/
function ingestBindings(
unit: ViewCompilationUnit, op: ir.ElementOpBase, element: t.Element|t.Template): void {
let flags: BindingFlags = BindingFlags.None;
const isPlainTemplate =
element instanceof t.Template && splitNsName(element.tagName ?? '')[1] === 'ng-template';
if (element instanceof t.Template) {
flags |= BindingFlags.OnNgTemplateElement;
if (isPlainTemplate) {
flags |= BindingFlags.BindingTargetsTemplate;
}
const templateAttrFlags =
flags | BindingFlags.BindingTargetsTemplate | BindingFlags.IsStructuralTemplateAttribute;
for (const attr of element.templateAttrs) {
if (attr instanceof t.TextAttribute) {
ingestBinding(
unit, op.xref, attr.name, o.literal(attr.value), e.BindingType.Attribute, null,
SecurityContext.NONE, attr.sourceSpan, true, true);
SecurityContext.NONE, attr.sourceSpan, templateAttrFlags | BindingFlags.TextValue);
} else {
ingestBinding(
unit, op.xref, attr.name, attr.value, attr.type, attr.unit, attr.securityContext,
attr.sourceSpan, false, true);
attr.sourceSpan, templateAttrFlags);
}
}
}
@ -505,13 +516,12 @@ function ingestBindings(
// `BindingType.Attribute`.
ingestBinding(
unit, op.xref, attr.name, o.literal(attr.value), e.BindingType.Attribute, null,
SecurityContext.NONE, attr.sourceSpan, true, false);
SecurityContext.NONE, attr.sourceSpan, flags | BindingFlags.TextValue);
}
for (const input of element.inputs) {
ingestBinding(
unit, op.xref, input.name, input.value, input.type, input.unit, input.securityContext,
input.sourceSpan, false, false);
input.sourceSpan, flags);
}
for (const output of element.outputs) {
@ -521,6 +531,13 @@ function ingestBindings(
throw Error('Animation listener should have a phase');
}
}
if (element instanceof t.Template && !isPlainTemplate) {
unit.create.push(
ir.createExtractedAttributeOp(op.xref, ir.BindingKind.Property, output.name, null));
continue;
}
listenerOp =
ir.createListenerOp(op.xref, output.name, op.tag, output.phase, false, output.sourceSpan);
@ -564,14 +581,46 @@ const BINDING_KINDS = new Map<e.BindingType, ir.BindingKind>([
[e.BindingType.Animation, ir.BindingKind.Animation],
]);
enum BindingFlags {
None = 0b000,
/**
* The binding is to a static text literal and not to an expression.
*/
TextValue = 0b0001,
/**
* The binding belongs to the `<ng-template>` side of a `t.Template`.
*/
BindingTargetsTemplate = 0b0010,
/**
* The binding is on a structural directive.
*/
IsStructuralTemplateAttribute = 0b0100,
/**
* The binding is on a `t.Template`.
*/
OnNgTemplateElement = 0b1000,
}
function ingestBinding(
view: ViewCompilationUnit, xref: ir.XrefId, name: string, value: e.AST|o.Expression,
type: e.BindingType, unit: string|null, securityContext: SecurityContext,
sourceSpan: ParseSourceSpan, isTextAttribute: boolean, isTemplateBinding: boolean): void {
sourceSpan: ParseSourceSpan, flags: BindingFlags): void {
if (value instanceof e.ASTWithSource) {
value = value.ast;
}
if (flags & BindingFlags.OnNgTemplateElement && !(flags & BindingFlags.BindingTargetsTemplate) &&
type === e.BindingType.Property) {
// This binding only exists for later const extraction, and is not an actual binding to be
// created.
view.create.push(ir.createExtractedAttributeOp(xref, ir.BindingKind.Property, name, null));
return;
}
let expression: o.Expression|ir.Interpolation;
// TODO: We could easily generate source maps for subexpressions in these cases, but
// TemplateDefinitionBuilder does not. Should we do so?
@ -586,8 +635,8 @@ function ingestBinding(
const kind: ir.BindingKind = BINDING_KINDS.get(type)!;
view.update.push(ir.createBindingOp(
xref, kind, name, expression, unit, securityContext, isTextAttribute, isTemplateBinding,
sourceSpan));
xref, kind, name, expression, unit, securityContext, !!(flags & BindingFlags.TextValue),
!!(flags & BindingFlags.IsStructuralTemplateAttribute), sourceSpan));
}
/**

View file

@ -1,35 +0,0 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import * as o from '../../../../output/output_ast';
import * as ir from '../../ir';
import type {CompilationJob} from '../compilation';
export function phaseNoListenersOnTemplates(job: CompilationJob): void {
for (const unit of job.units) {
let inTemplate = false;
for (const op of unit.create) {
switch (op.kind) {
case ir.OpKind.Template:
inTemplate = true;
break;
case ir.OpKind.ElementStart:
case ir.OpKind.Element:
case ir.OpKind.ContainerStart:
case ir.OpKind.Container:
inTemplate = false;
break;
case ir.OpKind.Listener:
if (inTemplate) {
ir.OpList.remove<ir.CreateOp>(op);
}
break;
}
}
}
}