Skip to content

Commit

Permalink
fix(compiler): correctly intercept index in loop tracking function (#…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
crisbeto authored and thePunderWoman committed Dec 18, 2023
1 parent 513fee8 commit de5c9ca
Show file tree
Hide file tree
Showing 5 changed files with 137 additions and 6 deletions.
Expand Up @@ -1926,3 +1926,54 @@ export declare class MyApp {
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
}

/****************************************************************************************************
* PARTIAL FILE: nested_for_tracking_function.js
****************************************************************************************************/
import { Component } from '@angular/core';
import * as i0 from "@angular/core";
export class MyApp {
constructor() {
this.items = [];
this.trackByGrandparent = (item, index) => index;
this.trackByParent = (item, index) => index;
this.trackByChild = (item, index) => index;
}
}
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 (grandparent of items; track trackByGrandparent(grandparent, $index)) {
@for (parent of grandparent.items; track trackByParent(parent, $index)) {
@for (child of parent.items; track trackByChild(child, $index)) {
}
}
}
`, isInline: true });
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: MyApp, decorators: [{
type: Component,
args: [{
template: `
@for (grandparent of items; track trackByGrandparent(grandparent, $index)) {
@for (parent of grandparent.items; track trackByParent(parent, $index)) {
@for (child of parent.items; track trackByChild(child, $index)) {
}
}
}
`,
}]
}] });

/****************************************************************************************************
* PARTIAL FILE: nested_for_tracking_function.d.ts
****************************************************************************************************/
import * as i0 from "@angular/core";
export declare class MyApp {
items: any[];
trackByGrandparent: (item: any, index: number) => number;
trackByParent: (item: any, index: number) => number;
trackByChild: (item: any, index: number) => number;
static ɵfac: i0.ɵɵFactoryDeclaration<MyApp, never>;
static ɵcmp: i0.ɵɵComponentDeclaration<MyApp, "ng-component", never, {}, {}, never, never, false, never>;
}

Expand Up @@ -617,6 +617,23 @@
}
],
"skipForTemplatePipeline": true
},
{
"description": "should generate tracking function in a nested for loop",
"inputFiles": [
"nested_for_tracking_function.ts"
],
"expectations": [
{
"files": [
{
"expected": "nested_for_tracking_function_template.js",
"generated": "nested_for_tracking_function.js"
}
],
"failureMessage": "Incorrect template"
}
]
}
]
}
@@ -0,0 +1,19 @@
import {Component} from '@angular/core';

@Component({
template: `
@for (grandparent of items; track trackByGrandparent(grandparent, $index)) {
@for (parent of grandparent.items; track trackByParent(parent, $index)) {
@for (child of parent.items; track trackByChild(child, $index)) {
}
}
}
`,
})
export class MyApp {
items: any[] = [];
trackByGrandparent = (item: any, index: number) => index;
trackByParent = (item: any, index: number) => index;
trackByChild = (item: any, index: number) => index;
}
@@ -0,0 +1,44 @@
function _forTrack0($index, $item) {
return this.trackByGrandparent($item, $index);
}

function _forTrack1($index, $item) {
return this.trackByParent($item, $index);
}

function _forTrack2($index, $item) {
return this.trackByChild($item, $index);
}

function MyApp_For_1_For_1_For_1_Template(rf, ctx) {}

function MyApp_For_1_For_1_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵrepeaterCreate(0, MyApp_For_1_For_1_For_1_Template, 0, 0, null, null, _forTrack2, true);
}
if (rf & 2) {
const $parent_r7$ = ctx.$implicit;
$r3$.ɵɵrepeater($parent_r7$.items);
}
}

function MyApp_For_1_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵrepeaterCreate(0, MyApp_For_1_For_1_Template, 2, 0, null, null, _forTrack1, true);
}
if (rf & 2) {
const $grandparent_r1$ = ctx.$implicit;
$r3$.ɵɵrepeater($grandparent_r1$.items);
}
}


function MyApp_Template(rf, ctx) {
if (rf & 1) {
$r3$.ɵɵrepeaterCreate(0, MyApp_For_1_Template, 2, 0, null, null, _forTrack0, true);
}
if (rf & 2) {
$r3$.ɵɵrepeater(ctx.items);
}
}
12 changes: 6 additions & 6 deletions packages/compiler/src/render3/view/template.ts
Expand Up @@ -2550,11 +2550,16 @@ export class BindingScope implements LocalResolver {
class TrackByBindingScope extends BindingScope {
private componentAccessCount = 0;

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

override get(name: string): o.Expression|null {
// Intercept any overridden globals.
if (this.globalOverrides.hasOwnProperty(name)) {
return o.variable(this.globalOverrides[name]);
}

let current: BindingScope|null = this.parent;

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

// Intercept any aliased globals.
if (this.globalAliases[name]) {
return o.variable(this.globalAliases[name]);
}

// When the component scope is accessed, we redirect it through `this`.
this.componentAccessCount++;
return o.variable('this').prop(name);
Expand Down

0 comments on commit de5c9ca

Please sign in to comment.