Skip to content

Commit

Permalink
fix(compiler): ng-template directive invoke twice at the root of cont…
Browse files Browse the repository at this point in the history
…rol flow (#52515)

Discovered this while validating #52414 against Angular Material. We were projecting `<ng-template>` nodes at the root of `@if` and `@for` with the `ng-template` tag name which enables directive matching and applies the directive to the control flow node.

These changes fix the issue by never passing along the `ng-template` tag name.

PR Close #52515
  • Loading branch information
crisbeto authored and atscott committed Nov 6, 2023
1 parent 09e905a commit 83067b3
Show file tree
Hide file tree
Showing 10 changed files with 240 additions and 2 deletions.
Expand Up @@ -1689,6 +1689,43 @@ export declare class MyApp {
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
}

/****************************************************************************************************
* PARTIAL FILE: if_template_root_node.js
****************************************************************************************************/
import { Component } from '@angular/core';
import * as i0 from "@angular/core";
export class MyApp {
constructor() {
this.expr = true;
}
}
MyApp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, deps: [], target: i0.ɵɵFactoryTarget.Component });
MyApp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "17.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, selector: "ng-component", ngImport: i0, template: `
@if (expr) {
<ng-template foo="1" bar="2">{{expr}}</ng-template>
}
`, isInline: true });
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{
type: Component,
args: [{
template: `
@if (expr) {
<ng-template foo="1" bar="2">{{expr}}</ng-template>
}
`,
}]
}] });

/****************************************************************************************************
* PARTIAL FILE: if_template_root_node.d.ts
****************************************************************************************************/
import * as i0 from "@angular/core";
export declare class MyApp {
expr: boolean;
static ɵfac: i0.ɵɵFactoryDeclaration<MyApp, never>;
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
}

/****************************************************************************************************
* PARTIAL FILE: for_element_root_node.js
****************************************************************************************************/
Expand Down Expand Up @@ -1726,3 +1763,40 @@ export declare class MyApp {
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
}

/****************************************************************************************************
* PARTIAL FILE: for_template_root_node.js
****************************************************************************************************/
import { Component } from '@angular/core';
import * as i0 from "@angular/core";
export class MyApp {
constructor() {
this.items = [1, 2, 3];
}
}
MyApp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, deps: [], target: i0.ɵɵFactoryTarget.Component });
MyApp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "17.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, selector: "ng-component", ngImport: i0, template: `
@for (item of items; track item) {
<ng-template foo="1" bar="2">{{item}}</ng-template>
}
`, isInline: true });
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{
type: Component,
args: [{
template: `
@for (item of items; track item) {
<ng-template foo="1" bar="2">{{item}}</ng-template>
}
`,
}]
}] });

/****************************************************************************************************
* PARTIAL FILE: for_template_root_node.d.ts
****************************************************************************************************/
import * as i0 from "@angular/core";
export declare class MyApp {
items: number[];
static ɵfac: i0.ɵɵFactoryDeclaration<MyApp, never>;
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
}

Expand Up @@ -531,6 +531,23 @@
}
]
},
{
"description": "should generate an if block with an ng-template root node",
"inputFiles": [
"if_template_root_node.ts"
],
"expectations": [
{
"files": [
{
"expected": "if_template_root_node_template.js",
"generated": "if_template_root_node.js"
}
],
"failureMessage": "Incorrect template"
}
]
},
{
"description": "should generate a for block with an element root node",
"inputFiles": [
Expand All @@ -547,6 +564,23 @@
"failureMessage": "Incorrect template"
}
]
},
{
"description": "should generate a for block with an ng-template root node",
"inputFiles": [
"for_template_root_node.ts"
],
"expectations": [
{
"files": [
{
"expected": "for_template_root_node_template.js",
"generated": "for_template_root_node.js"
}
],
"failureMessage": "Incorrect template"
}
]
}
]
}
@@ -0,0 +1,12 @@
import {Component} from '@angular/core';

@Component({
template: `
@for (item of items; track item) {
<ng-template foo="1" bar="2">{{item}}</ng-template>
}
`,
})
export class MyApp {
items = [1, 2, 3];
}
@@ -0,0 +1,3 @@
consts: [["foo", "1", "bar", "2"]]
$r3$.ɵɵrepeaterCreate(0, MyApp_For_1_Template, 1, 0, null, 0, i0.ɵɵrepeaterTrackByIdentity);
@@ -0,0 +1,12 @@
import {Component} from '@angular/core';

@Component({
template: `
@if (expr) {
<ng-template foo="1" bar="2">{{expr}}</ng-template>
}
`,
})
export class MyApp {
expr = true;
}
@@ -0,0 +1,3 @@
consts: [["foo", "1", "bar", "2"]]
$r3$.ɵɵtemplate(0, MyApp_Conditional_0_Template, 1, 0, null, 0);
4 changes: 3 additions & 1 deletion packages/compiler/src/render3/view/template.ts
Expand Up @@ -1508,7 +1508,9 @@ export class TemplateDefinitionBuilder implements t.Visitor<void>, LocalResolver
// that we don't copy any bound attributes since they don't participate in content projection
// and they can be used in directive matching (in the case of `Template.templateAttrs`).
if (root !== null) {
tagName = root instanceof t.Element ? root.name : root.tagName;
const name = root instanceof t.Element ? root.name : root.tagName;
// Don't pass along `ng-template` tag name since it enables directive matching.
tagName = name === NG_TEMPLATE_TAG_NAME ? null : name;
attrsExprs =
this.getAttributeExpressions(NG_TEMPLATE_TAG_NAME, root.attributes, root.inputs, []);
}
Expand Down
5 changes: 4 additions & 1 deletion packages/compiler/src/template/pipeline/src/ingest.ts
Expand Up @@ -907,7 +907,10 @@ function ingestControlFlowInsertionPoint(
SecurityContext.NONE, attr.sourceSpan, BindingFlags.TextValue);
}

return root instanceof t.Element ? root.name : root.tagName;
const tagName = root instanceof t.Element ? root.name : root.tagName;

// Don't pass along `ng-template` tag name since it enables directive matching.
return tagName === 'ng-template' ? null : tagName;
}

return null;
Expand Down
48 changes: 48 additions & 0 deletions packages/core/test/acceptance/control_flow_for_spec.ts
Expand Up @@ -572,5 +572,53 @@ describe('control flow - for', () => {
expect(directiveCount).toBe(1);
expect(fixture.nativeElement.textContent).toBe('Main: Before After Slot: 1');
});

it('should invoke a directive on a projected ng-template at the root of an @for once', () => {
let directiveCount = 0;

@Component({
standalone: true,
selector: 'test',
template: 'Main: <ng-content/> Slot: <ng-content select="[foo]"/>',
})
class TestComponent {
}

@Directive({
selector: '[templateDir]',
standalone: true,
})
class TemplateDirective implements OnInit {
constructor(
private viewContainerRef: ViewContainerRef,
private templateRef: TemplateRef<any>,
) {
directiveCount++;
}

ngOnInit(): void {
const view = this.viewContainerRef.createEmbeddedView(this.templateRef);
this.viewContainerRef.insert(view);
}
}

@Component({
standalone: true,
imports: [TestComponent, TemplateDirective],
template: `<test>Before @for (item of items; track $index) {
<ng-template templateDir foo>{{item}}</ng-template>
} After</test>
`
})
class App {
items = [1];
}

const fixture = TestBed.createComponent(App);
fixture.detectChanges();

expect(directiveCount).toBe(1);
expect(fixture.nativeElement.textContent).toBe('Main: Before After Slot: 1');
});
});
});
47 changes: 47 additions & 0 deletions packages/core/test/acceptance/control_flow_if_spec.ts
Expand Up @@ -582,5 +582,52 @@ describe('control flow - if', () => {
expect(directiveCount).toBe(1);
expect(fixture.nativeElement.textContent).toBe('Main: Before After Slot: foo');
});

it('should invoke a directive on a projected ng-template at the root of an @if once', () => {
let directiveCount = 0;

@Component({
standalone: true,
selector: 'test',
template: 'Main: <ng-content/> Slot: <ng-content select="[foo]"/>',
})
class TestComponent {
}

@Directive({
selector: '[templateDir]',
standalone: true,
})
class TemplateDirective implements OnInit {
constructor(
private viewContainerRef: ViewContainerRef,
private templateRef: TemplateRef<any>,
) {
directiveCount++;
}

ngOnInit(): void {
const view = this.viewContainerRef.createEmbeddedView(this.templateRef);
this.viewContainerRef.insert(view);
}
}

@Component({
standalone: true,
imports: [TestComponent, TemplateDirective],
template: `<test>Before @if (true) {
<ng-template templateDir foo>foo</ng-template>
} After</test>
`
})
class App {
}

const fixture = TestBed.createComponent(App);
fixture.detectChanges();

expect(directiveCount).toBe(1);
expect(fixture.nativeElement.textContent).toBe('Main: Before After Slot: foo');
});
});
});

0 comments on commit 83067b3

Please sign in to comment.