Skip to content

Commit 5ae2bf4

Browse files
crisbetoatscott
crisbeto
authored and
atscott
committed
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
1 parent ffb9b44 commit 5ae2bf4

File tree

7 files changed

+180
-3
lines changed

7 files changed

+180
-3
lines changed

‎packages/compiler-cli/src/ngtsc/typecheck/api/checker.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,7 @@ export interface TemplateTypeChecker {
215215

216216
/**
217217
* Gets the target of a template expression, if possible.
218+
* See `BoundTarget.getExpressionTarget` for more information.
218219
*/
219220
getExpressionTarget(expression: AST, clazz: ts.ClassDeclaration): TmplAstReference|TmplAstVariable
220221
|null;

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

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -917,3 +917,61 @@ export declare class App {
917917
static ɵcmp: i0.ɵɵComponentDeclaration<App, "ng-component", never, {}, {}, never, never, true, never>;
918918
}
919919

920+
/****************************************************************************************************
921+
* PARTIAL FILE: two_way_binding_to_signal_loop_variable.js
922+
****************************************************************************************************/
923+
import { Component, Directive, model, signal } from '@angular/core';
924+
import * as i0 from "@angular/core";
925+
export class NgModelDirective {
926+
constructor() {
927+
this.ngModel = model.required();
928+
}
929+
}
930+
NgModelDirective.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: NgModelDirective, deps: [], target: i0.ɵɵFactoryTarget.Directive });
931+
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 });
932+
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: NgModelDirective, decorators: [{
933+
type: Directive,
934+
args: [{
935+
selector: '[ngModel]',
936+
standalone: true,
937+
}]
938+
}] });
939+
export class TestCmp {
940+
constructor() {
941+
this.names = [signal('Angular')];
942+
}
943+
}
944+
TestCmp.ɵfac = i0.ɵɵngDeclareFactory({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestCmp, deps: [], target: i0.ɵɵFactoryTarget.Component });
945+
TestCmp.ɵcmp = i0.ɵɵngDeclareComponent({ minVersion: "17.0.0", version: "0.0.0-PLACEHOLDER", type: TestCmp, isStandalone: true, selector: "ng-component", ngImport: i0, template: `
946+
@for (name of names; track $index) {
947+
<input [(ngModel)]="name" />
948+
}
949+
`, isInline: true, dependencies: [{ kind: "directive", type: NgModelDirective, selector: "[ngModel]", inputs: ["ngModel"], outputs: ["ngModelChange"] }] });
950+
i0.ɵɵngDeclareClassMetadata({ minVersion: "12.0.0", version: "0.0.0-PLACEHOLDER", ngImport: i0, type: TestCmp, decorators: [{
951+
type: Component,
952+
args: [{
953+
template: `
954+
@for (name of names; track $index) {
955+
<input [(ngModel)]="name" />
956+
}
957+
`,
958+
standalone: true,
959+
imports: [NgModelDirective],
960+
}]
961+
}] });
962+
963+
/****************************************************************************************************
964+
* PARTIAL FILE: two_way_binding_to_signal_loop_variable.d.ts
965+
****************************************************************************************************/
966+
import * as i0 from "@angular/core";
967+
export declare class NgModelDirective {
968+
ngModel: import("@angular/core").ModelSignal<string>;
969+
static ɵfac: i0.ɵɵFactoryDeclaration<NgModelDirective, never>;
970+
static ɵdir: i0.ɵɵDirectiveDeclaration<NgModelDirective, "[ngModel]", never, { "ngModel": { "alias": "ngModel"; "required": true; "isSignal": true; }; }, { "ngModel": "ngModelChange"; }, never, never, true, never>;
971+
}
972+
export declare class TestCmp {
973+
names: import("@angular/core").WritableSignal<string>[];
974+
static ɵfac: i0.ɵɵFactoryDeclaration<TestCmp, never>;
975+
static ɵcmp: i0.ɵɵComponentDeclaration<TestCmp, "ng-component", never, {}, {}, never, never, true, never>;
976+
}
977+

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

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -353,6 +353,23 @@
353353
"failureMessage": "Incorrect template"
354354
}
355355
]
356+
},
357+
{
358+
"description": "should generate a two-way binding to a @for loop variable that is a signal",
359+
"inputFiles": [
360+
"two_way_binding_to_signal_loop_variable.ts"
361+
],
362+
"expectations": [
363+
{
364+
"files": [
365+
{
366+
"expected": "two_way_binding_to_signal_loop_variable_template.js",
367+
"generated": "two_way_binding_to_signal_loop_variable.js"
368+
}
369+
],
370+
"failureMessage": "Incorrect template"
371+
}
372+
]
356373
}
357374
]
358375
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/two_way_binding_to_signal_loop_variable.ts‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
import {Component, Directive, model, signal} from '@angular/core';
2+
3+
@Directive({
4+
selector: '[ngModel]',
5+
standalone: true,
6+
})
7+
export class NgModelDirective {
8+
ngModel = model.required<string>();
9+
}
10+
11+
@Component({
12+
template: `
13+
@for (name of names; track $index) {
14+
<input [(ngModel)]="name" />
15+
}
16+
`,
17+
standalone: true,
18+
imports: [NgModelDirective],
19+
})
20+
export class TestCmp {
21+
names = [signal('Angular')];
22+
}

‎packages/compiler-cli/test/compliance/test_cases/r3_view_compiler_listener/two_way_binding_to_signal_loop_variable_template.js‎

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
function TestCmp_For_1_Template(rf, ctx) {
2+
if (rf & 1) {
3+
const $_r1$ = $r3$.ɵɵgetCurrentView();
4+
$r3$.ɵɵelementStart(0, "input", 0);
5+
$r3$.ɵɵtwoWayListener("ngModelChange", function TestCmp_For_1_Template_input_ngModelChange_0_listener($event) {
6+
const $name_r2$ = $r3$.ɵɵrestoreView($_r1$).$implicit;
7+
$r3$.ɵɵtwoWayBindingSet($name_r2$, $event);
8+
return $r3$.ɵɵresetView($event);
9+
});
10+
$r3$.ɵɵelementEnd();
11+
}
12+
if (rf & 2) {
13+
const $name_r2$ = ctx.$implicit;
14+
$r3$.ɵɵtwoWayProperty("ngModel", $name_r2$);
15+
}
16+
}
17+
18+
…
19+
20+
function TestCmp_Template(rf, ctx) {
21+
if (rf & 1) {
22+
$r3$.ɵɵrepeaterCreate(0, TestCmp_For_1_Template, 1, 1, "input", null, $r3$.ɵɵrepeaterTrackByIndex);
23+
}
24+
if (rf & 2) {
25+
$r3$.ɵɵrepeater(ctx.names);
26+
}
27+
}

‎packages/compiler/src/template/pipeline/src/phases/transform_two_way_binding_set.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -32,12 +32,24 @@ export function transformTwoWayBindingSet(job: CompilationJob): void {
3232
}
3333
}
3434

35-
function wrapSetOperation(target: o.ReadPropExpr|o.ReadKeyExpr, value: o.Expression): o.Expression {
35+
function wrapSetOperation(
36+
target: o.ReadPropExpr|o.ReadKeyExpr|ir.ReadVariableExpr, value: o.Expression): o.Expression {
37+
// ASSUMPTION: here we're assuming that `ReadVariableExpr` will be a reference
38+
// to a local template variable. This appears to be the case at the time of writing.
39+
// If the expression is targeting a variable read, we only emit the `twoWayBindingSet` since
40+
// the fallback would be attempting to write into a constant. Invalid usages will be flagged
41+
// during template type checking.
42+
if (target instanceof ir.ReadVariableExpr) {
43+
return ng.twoWayBindingSet(target, value);
44+
}
45+
3646
return ng.twoWayBindingSet(target, value).or(target.set(value));
3747
}
3848

39-
function isReadExpression(value: unknown): value is o.ReadPropExpr|o.ReadKeyExpr {
40-
return value instanceof o.ReadPropExpr || value instanceof o.ReadKeyExpr;
49+
function isReadExpression(value: unknown): value is o.ReadPropExpr|o.ReadKeyExpr|
50+
ir.ReadVariableExpr {
51+
return value instanceof o.ReadPropExpr || value instanceof o.ReadKeyExpr ||
52+
value instanceof ir.ReadVariableExpr;
4153
}
4254

4355
function wrapAction(target: o.Expression, value: o.Expression): o.Expression {

‎packages/core/test/acceptance/authoring/model_inputs_spec.ts‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -589,4 +589,44 @@ describe('model inputs', () => {
589589
fixture.detectChanges();
590590
expect(() => fixture.destroy()).not.toThrow();
591591
});
592+
593+
it('should support two-way binding to a signal @for loop variable', () => {
594+
@Directive({selector: '[dir]', standalone: true})
595+
class Dir {
596+
value = model(0);
597+
}
598+
599+
@Component({
600+
template: `
601+
@for (value of values; track $index) {
602+
<div [(value)]="value" dir></div>
603+
}
604+
`,
605+
standalone: true,
606+
imports: [Dir],
607+
})
608+
class App {
609+
@ViewChild(Dir) dir!: Dir;
610+
values = [signal(1)];
611+
}
612+
613+
const fixture = TestBed.createComponent(App);
614+
const host = fixture.componentInstance;
615+
fixture.detectChanges();
616+
617+
// Initial value.
618+
expect(host.values[0]()).toBe(1);
619+
expect(host.dir.value()).toBe(1);
620+
621+
// Changing the value from within the directive.
622+
host.dir.value.set(2);
623+
expect(host.values[0]()).toBe(2);
624+
expect(host.dir.value()).toBe(2);
625+
626+
// Changing the value from the outside.
627+
host.values[0].set(3);
628+
fixture.detectChanges();
629+
expect(host.values[0]()).toBe(3);
630+
expect(host.dir.value()).toBe(3);
631+
});
592632
});

0 commit comments

Comments
 (0)