fix(compiler): invoke method-based tracking function with context (#54960)

crisbeto · dylhunn · commit 2b7bad515100 · 2024-03-21T22:08:41.000-07:00
Previously we assumed that if a `for` loop tracking function is in the form of `someMethod($index, $item)`, it will be pure so we didn't pass the parameter to bind the context to it. This appears to be risky, because we don't know if the method is trying to access `this`. These changes play it safe by always binding method-based tracking functions. Fixes #53628. PR Close #54960
diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_template_track_method_nested_template.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_template_track_method_nested_template.js
@@ -1 +1 @@
-$r3$.ɵɵrepeaterCreate(0, MyApp_ng_template_2_For_1_Template, 0, 0, null, null, $r3$.ɵɵcomponentInstance().trackFn);
+$r3$.ɵɵrepeaterCreate(0, MyApp_ng_template_2_For_1_Template, 0, 0, null, null, $r3$.ɵɵcomponentInstance().trackFn, true);
diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_template_track_method_root_template.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_control_flow/for_template_track_method_root_template.js
@@ -1 +1 @@
-$r3$.ɵɵrepeaterCreate(2, MyApp_For_3_Template, 0, 0, null, null, ctx.trackFn);
+$r3$.ɵɵrepeaterCreate(2, MyApp_For_3_Template, 0, 0, null, null, ctx.trackFn, true);
diff --git a/packages/compiler/src/template/pipeline/src/phases/track_fn_optimization.ts b/packages/compiler/src/template/pipeline/src/phases/track_fn_optimization.ts
@@ -31,6 +31,10 @@ export function optimizeTrackFns(job: CompilationJob): void {
         // Top-level access of the item uses the built in `repeaterTrackByIdentity`.
         op.trackByFn = o.importExpr(Identifiers.repeaterTrackByIdentity);
       } else if (isTrackByFunctionCall(job.root.xref, op.track)) {
+        // Mark the function as using the component instance to play it safe
+        // since the method might be using `this` internally (see #53628).
+        op.usesComponentInstance = true;
+
         // Top-level method calls in the form of `fn($index, item)` can be passed in directly.
         if (op.track.receiver.receiver.view === unit.xref) {
           // TODO: this may be wrong
diff --git a/packages/core/test/acceptance/control_flow_for_spec.ts b/packages/core/test/acceptance/control_flow_for_spec.ts
@@ -235,6 +235,26 @@ describe('control flow - for', () => {
          fixture.detectChanges();
          expect([...calls].sort()).toEqual(['a:hello', 'b:hello']);
        });
+
+    it('should invoke method tracking function with the correct context', () => {
+      let context = null as TestComponent | null;
+
+      @Component({
+        template: `@for (item of items; track trackingFn($index, item)) {{{item}}}`,
+      })
+      class TestComponent {
+        items = ['a', 'b'];
+
+        trackingFn(_index: number, item: string) {
+          context = this;
+          return item;
+        }
+      }
+
+      const fixture = TestBed.createComponent(TestComponent);
+      fixture.detectChanges();
+      expect(context).toBe(fixture.componentInstance);
+    });
   });
 
   describe('list diffing and view operations', () => {

Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-$r3$.ɵɵrepeaterCreate(0, MyApp_ng_template_2_For_1_Template, 0, 0, null, null, $r3$.ɵɵcomponentInstance().trackFn);`
	`1`	`+$r3$.ɵɵrepeaterCreate(0, MyApp_ng_template_2_For_1_Template, 0, 0, null, null, $r3$.ɵɵcomponentInstance().trackFn, true);`
Original file line number	Diff line number	Diff line change
`@@ -1 +1 @@`
`1`		`-$r3$.ɵɵrepeaterCreate(2, MyApp_For_3_Template, 0, 0, null, null, ctx.trackFn);`
	`1`	`+$r3$.ɵɵrepeaterCreate(2, MyApp_For_3_Template, 0, 0, null, null, ctx.trackFn, true);`