Skip to content

Commit de5c9ca

Browse files
crisbetothePunderWoman
crisbeto
authored and
thePunderWoman
committed
fix(compiler): correctly intercept index in loop tracking function (#53604)
The for loop tracking function doesn't allow references to local template variables, aside from `$index` and the item which are passed in as parameters. We enforce this by rewriting all variable references to the components scope. The problem is that the logic that rewrites the references first walks the view tree and then checks if the variable is `$index` or the item. This is problematic in nested for loops, because it'll find the `$index` of the parent. These changes resolve the issue by checking for `$index` and the item first. Fixes #53600. PR Close #53604
1 parent 513fee8 commit de5c9ca

File tree

5 files changed

+137
-6
lines changed

5 files changed

+137
-6
lines changed

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/GOLDEN_PARTIAL.js‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1926,3 +1926,54 @@ export declare class MyApp {
19261926
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
19271927
}
19281928

1929+
/****************************************************************************************************
1930+
* PARTIAL FILE: nested_for_tracking_function.js
1931+
****************************************************************************************************/
1932+
import { Component } from '@angular/core';
1933+
import * as i0 from "@angular/core";
1934+
export class MyApp {
1935+
constructor() {
1936+
this.items = [];
1937+
this.trackByGrandparent = (item, index) => index;
1938+
this.trackByParent = (item, index) => index;
1939+
this.trackByChild = (item, index) => index;
1940+
}
1941+
}
1942+
MyApp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, deps: [], target: i0.ɵɵFactoryTarget.Component });
1943+
MyApp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "17.0.0", version: "0.0.0-PLACEHOLDER", type: MyApp, selector: "ng-component", ngImport: i0, template: `
1944+
@for (grandparent of items; track trackByGrandparent(grandparent, $index)) {
1945+
@for (parent of grandparent.items; track trackByParent(parent, $index)) {
1946+
@for (child of parent.items; track trackByChild(child, $index)) {
1947+
1948+
}
1949+
}
1950+
}
1951+
`, isInline: true });
1952+
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{
1953+
type: Component,
1954+
args: [{
1955+
template: `
1956+
@for (grandparent of items; track trackByGrandparent(grandparent, $index)) {
1957+
@for (parent of grandparent.items; track trackByParent(parent, $index)) {
1958+
@for (child of parent.items; track trackByChild(child, $index)) {
1959+
1960+
}
1961+
}
1962+
}
1963+
`,
1964+
}]
1965+
}] });
1966+
1967+
/****************************************************************************************************
1968+
* PARTIAL FILE: nested_for_tracking_function.d.ts
1969+
****************************************************************************************************/
1970+
import * as i0 from "@angular/core";
1971+
export declare class MyApp {
1972+
items: any[];
1973+
trackByGrandparent: (item: any, index: number) => number;
1974+
trackByParent: (item: any, index: number) => number;
1975+
trackByChild: (item: any, index: number) => number;
1976+
static ɵfac: i0.ɵɵFactoryDeclaration<MyApp, never>;
1977+
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
1978+
}
1979+

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/TEST_CASES.json‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -617,6 +617,23 @@
617617
}
618618
],
619619
"skipForTemplatePipeline": true
620+
},
621+
{
622+
"description": "should generate tracking function in a nested for loop",
623+
"inputFiles": [
624+
"nested_for_tracking_function.ts"
625+
],
626+
"expectations": [
627+
{
628+
"files": [
629+
{
630+
"expected": "nested_for_tracking_function_template.js",
631+
"generated": "nested_for_tracking_function.js"
632+
}
633+
],
634+
"failureMessage": "Incorrect template"
635+
}
636+
]
620637
}
621638
]
622639
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_tracking_function.ts‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
import {Component} from '@angular/core';
2+
3+
@Component({
4+
template: `
5+
@for (grandparent of items; track trackByGrandparent(grandparent, $index)) {
6+
@for (parent of grandparent.items; track trackByParent(parent, $index)) {
7+
@for (child of parent.items; track trackByChild(child, $index)) {
8+
9+
}
10+
}
11+
}
12+
`,
13+
})
14+
export class MyApp {
15+
items: any[] = [];
16+
trackByGrandparent = (item: any, index: number) => index;
17+
trackByParent = (item: any, index: number) => index;
18+
trackByChild = (item: any, index: number) => index;
19+
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/nested_for_tracking_function_template.js‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
function _forTrack0($index, $item) {
2+
return this.trackByGrandparent($item, $index);
3+
}
4+
5+
function _forTrack1($index, $item) {
6+
return this.trackByParent($item, $index);
7+
}
8+
9+
function _forTrack2($index, $item) {
10+
return this.trackByChild($item, $index);
11+
}
12+
13+
function MyApp_For_1_For_1_For_1_Template(rf, ctx) {}
14+
15+
function MyApp_For_1_For_1_Template(rf, ctx) {
16+
if (rf & 1) {
17+
$r3$.ɵɵrepeaterCreate(0, MyApp_For_1_For_1_For_1_Template, 0, 0, null, null, _forTrack2, true);
18+
}
19+
if (rf & 2) {
20+
const $parent_r7$ = ctx.$implicit;
21+
$r3$.ɵɵrepeater($parent_r7$.items);
22+
}
23+
}
24+
25+
function MyApp_For_1_Template(rf, ctx) {
26+
if (rf & 1) {
27+
$r3$.ɵɵrepeaterCreate(0, MyApp_For_1_For_1_Template, 2, 0, null, null, _forTrack1, true);
28+
}
29+
if (rf & 2) {
30+
const $grandparent_r1$ = ctx.$implicit;
31+
$r3$.ɵɵrepeater($grandparent_r1$.items);
32+
}
33+
}
34+
35+
…
36+
37+
function MyApp_Template(rf, ctx) {
38+
if (rf & 1) {
39+
$r3$.ɵɵrepeaterCreate(0, MyApp_For_1_Template, 2, 0, null, null, _forTrack0, true);
40+
}
41+
if (rf & 2) {
42+
$r3$.ɵɵrepeater(ctx.items);
43+
}
44+
}

‎packages/compiler/src/render3/view/template.ts‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2550,11 +2550,16 @@ export class BindingScope implements LocalResolver {
25502550
class TrackByBindingScope extends BindingScope {
25512551
private componentAccessCount = 0;
25522552

2553-
constructor(parentScope: BindingScope, private globalAliases: Record<string, string>) {
2553+
constructor(parentScope: BindingScope, private globalOverrides: Record<string, string>) {
25542554
super(parentScope.bindingLevel + 1, parentScope);
25552555
}
25562556

25572557
override get(name: string): o.Expression|null {
2558+
// Intercept any overridden globals.
2559+
if (this.globalOverrides.hasOwnProperty(name)) {
2560+
return o.variable(this.globalOverrides[name]);
2561+
}
2562+
25582563
let current: BindingScope|null = this.parent;
25592564

25602565
// Prevent accesses of template variables outside the `for` loop.
@@ -2565,11 +2570,6 @@ class TrackByBindingScope extends BindingScope {
25652570
current = current.parent;
25662571
}
25672572

2568-
// Intercept any aliased globals.
2569-
if (this.globalAliases[name]) {
2570-
return o.variable(this.globalAliases[name]);
2571-
}
2572-
25732573
// When the component scope is accessed, we redirect it through `this`.
25742574
this.componentAccessCount++;
25752575
return o.variable('this').prop(name);

0 commit comments

Comments
 (0)