From 5ae2bf480697b475c41ea136f08f4dca633c75b9 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 8 Mar 2024 13:39:57 +0100 Subject: [PATCH] fix(compiler): handle two-way bindings to signal-based template variables in instruction generation (#54714) Updates the instruction generation for two-way bindings to only emit the `twoWayBindingSet` call when writing to template variables. Since template variables are constants, it's only allowed to write to them when they're signals. Non-signal values are flagged during template type checking. Fixes #54670. PR Close #54714 --- .../src/ngtsc/typecheck/api/checker.ts | 1 + .../GOLDEN_PARTIAL.js | 58 +++++++++++++++++++ .../r3_view_compiler_listener/TEST_CASES.json | 17 ++++++ ...two_way_binding_to_signal_loop_variable.ts | 22 +++++++ ...inding_to_signal_loop_variable_template.js | 27 +++++++++ .../phases/transform_two_way_binding_set.ts | 18 +++++- .../acceptance/authoring/model_inputs_spec.ts | 40 +++++++++++++ 7 files changed, 180 insertions(+), 3 deletions(-) create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/two_way_binding_to_signal_loop_variable.ts create mode 100644 packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/two_way_binding_to_signal_loop_variable_template.js diff --git a/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts b/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts index 71bea4d3c07..bccff7774fd 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts @@ -215,6 +215,7 @@ export interface TemplateTypeChecker { /** * Gets the target of a template expression, if possible. + * See `BoundTarget.getExpressionTarget` for more information. */ getExpressionTarget(expression: AST, clazz: ts.ClassDeclaration): TmplAstReference|TmplAstVariable |null; diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js index 1d298f51ea4..f1365f5813c 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/GOLDEN_PARTIAL.js @@ -917,3 +917,61 @@ export declare class App { static ɵcmp: i0.ɵɵComponentDeclaration; } +/**************************************************************************************************** + * PARTIAL FILE: two_way_binding_to_signal_loop_variable.js + ****************************************************************************************************/ +import { Component, Directive, model, signal } from '@angular/core'; +import * as i0 from "@angular/core"; +export class NgModelDirective { + constructor() { + this.ngModel = model.required(); + } +} +NgModelDirective.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: NgModelDirective, deps: [], target: i0.ɵɵFactoryTarget.Directive }); +NgModelDirective.ɵdir = i0.ɵɵngDeclareDirective({ minVersion: "17.1.0", version: "0.0.0-PLACEHOLDER", type: NgModelDirective, isStandalone: true, selector: "[ngModel]", inputs: { ngModel: { classPropertyName: "ngModel", publicName: "ngModel", isSignal: true, isRequired: true, transformFunction: null } }, outputs: { ngModel: "ngModelChange" }, ngImport: i0 }); +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: NgModelDirective, decorators: [{ + type: Directive, + args: [{ + selector: '[ngModel]', + standalone: true, + }] + }] }); +export class TestCmp { + constructor() { + this.names = [signal('Angular')]; + } +} +TestCmp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestCmp, deps: [], target: i0.ɵɵFactoryTarget.Component }); +TestCmp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "17.0.0", version: "0.0.0-PLACEHOLDER", type: TestCmp, isStandalone: true, selector: "ng-component", ngImport: i0, template: ` + @for (name of names; track $index) { + + } + `, isInline: true, dependencies: [{ kind: "directive", type: NgModelDirective, selector: "[ngModel]", inputs: ["ngModel"], outputs: ["ngModelChange"] }] }); +i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestCmp, decorators: [{ + type: Component, + args: [{ + template: ` + @for (name of names; track $index) { + + } + `, + standalone: true, + imports: [NgModelDirective], + }] + }] }); + +/**************************************************************************************************** + * PARTIAL FILE: two_way_binding_to_signal_loop_variable.d.ts + ****************************************************************************************************/ +import * as i0 from "@angular/core"; +export declare class NgModelDirective { + ngModel: import("@angular/core").ModelSignal; + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵdir: i0.ɵɵDirectiveDeclaration; +} +export declare class TestCmp { + names: import("@angular/core").WritableSignal[]; + static ɵfac: i0.ɵɵFactoryDeclaration; + static ɵcmp: i0.ɵɵComponentDeclaration; +} + diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json index 6daf5fc2ddf..87e8d65c79a 100644 --- a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/TEST_CASES.json @@ -353,6 +353,23 @@ "failureMessage": "Incorrect template" } ] + }, + { + "description": "should generate a two-way binding to a @for loop variable that is a signal", + "inputFiles": [ + "two_way_binding_to_signal_loop_variable.ts" + ], + "expectations": [ + { + "files": [ + { + "expected": "two_way_binding_to_signal_loop_variable_template.js", + "generated": "two_way_binding_to_signal_loop_variable.js" + } + ], + "failureMessage": "Incorrect template" + } + ] } ] } diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/two_way_binding_to_signal_loop_variable.ts b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/two_way_binding_to_signal_loop_variable.ts new file mode 100644 index 00000000000..3a28d1f23dc --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/two_way_binding_to_signal_loop_variable.ts @@ -0,0 +1,22 @@ +import {Component, Directive, model, signal} from '@angular/core'; + +@Directive({ + selector: '[ngModel]', + standalone: true, +}) +export class NgModelDirective { + ngModel = model.required(); +} + +@Component({ + template: ` + @for (name of names; track $index) { + + } + `, + standalone: true, + imports: [NgModelDirective], +}) +export class TestCmp { + names = [signal('Angular')]; +} diff --git a/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/two_way_binding_to_signal_loop_variable_template.js b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/two_way_binding_to_signal_loop_variable_template.js new file mode 100644 index 00000000000..8dee4c7d9a1 --- /dev/null +++ b/packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/two_way_binding_to_signal_loop_variable_template.js @@ -0,0 +1,27 @@ +function TestCmp_For_1_Template(rf, ctx) { + if (rf & 1) { + const $_r1$ = $r3$.ɵɵgetCurrentView(); + $r3$.ɵɵelementStart(0, "input", 0); + $r3$.ɵɵtwoWayListener("ngModelChange", function TestCmp_For_1_Template_input_ngModelChange_0_listener($event) { + const $name_r2$ = $r3$.ɵɵrestoreView($_r1$).$implicit; + $r3$.ɵɵtwoWayBindingSet($name_r2$, $event); + return $r3$.ɵɵresetView($event); + }); + $r3$.ɵɵelementEnd(); + } + if (rf & 2) { + const $name_r2$ = ctx.$implicit; + $r3$.ɵɵtwoWayProperty("ngModel", $name_r2$); + } +} + +… + +function TestCmp_Template(rf, ctx) { + if (rf & 1) { + $r3$.ɵɵrepeaterCreate(0, TestCmp_For_1_Template, 1, 1, "input", null, $r3$.ɵɵrepeaterTrackByIndex); + } + if (rf & 2) { + $r3$.ɵɵrepeater(ctx.names); + } +} diff --git a/packages/compiler/src/template/pipeline/src/phases/transform_two_way_binding_set.ts b/packages/compiler/src/template/pipeline/src/phases/transform_two_way_binding_set.ts index f94dbbe8642..ba308d03784 100644 --- a/packages/compiler/src/template/pipeline/src/phases/transform_two_way_binding_set.ts +++ b/packages/compiler/src/template/pipeline/src/phases/transform_two_way_binding_set.ts @@ -32,12 +32,24 @@ export function transformTwoWayBindingSet(job: CompilationJob): void { } } -function wrapSetOperation(target: o.ReadPropExpr|o.ReadKeyExpr, value: o.Expression): o.Expression { +function wrapSetOperation( + target: o.ReadPropExpr|o.ReadKeyExpr|ir.ReadVariableExpr, value: o.Expression): o.Expression { + // ASSUMPTION: here we're assuming that `ReadVariableExpr` will be a reference + // to a local template variable. This appears to be the case at the time of writing. + // If the expression is targeting a variable read, we only emit the `twoWayBindingSet` since + // the fallback would be attempting to write into a constant. Invalid usages will be flagged + // during template type checking. + if (target instanceof ir.ReadVariableExpr) { + return ng.twoWayBindingSet(target, value); + } + return ng.twoWayBindingSet(target, value).or(target.set(value)); } -function isReadExpression(value: unknown): value is o.ReadPropExpr|o.ReadKeyExpr { - return value instanceof o.ReadPropExpr || value instanceof o.ReadKeyExpr; +function isReadExpression(value: unknown): value is o.ReadPropExpr|o.ReadKeyExpr| + ir.ReadVariableExpr { + return value instanceof o.ReadPropExpr || value instanceof o.ReadKeyExpr || + value instanceof ir.ReadVariableExpr; } function wrapAction(target: o.Expression, value: o.Expression): o.Expression { diff --git a/packages/core/test/acceptance/authoring/model_inputs_spec.ts b/packages/core/test/acceptance/authoring/model_inputs_spec.ts index 6427b226225..3750868da2a 100644 --- a/packages/core/test/acceptance/authoring/model_inputs_spec.ts +++ b/packages/core/test/acceptance/authoring/model_inputs_spec.ts @@ -589,4 +589,44 @@ describe('model inputs', () => { fixture.detectChanges(); expect(() => fixture.destroy()).not.toThrow(); }); + + it('should support two-way binding to a signal @for loop variable', () => { + @Directive({selector: '[dir]', standalone: true}) + class Dir { + value = model(0); + } + + @Component({ + template: ` + @for (value of values; track $index) { +
+ } + `, + standalone: true, + imports: [Dir], + }) + class App { + @ViewChild(Dir) dir!: Dir; + values = [signal(1)]; + } + + const fixture = TestBed.createComponent(App); + const host = fixture.componentInstance; + fixture.detectChanges(); + + // Initial value. + expect(host.values[0]()).toBe(1); + expect(host.dir.value()).toBe(1); + + // Changing the value from within the directive. + host.dir.value.set(2); + expect(host.values[0]()).toBe(2); + expect(host.dir.value()).toBe(2); + + // Changing the value from the outside. + host.values[0].set(3); + fixture.detectChanges(); + expect(host.values[0]()).toBe(3); + expect(host.dir.value()).toBe(3); + }); });