Skip to content

Commit 67b977e

Browse files
crisbetothePunderWoman
crisbeto
authored and
thePunderWoman
committed
refactor(compiler-cli): allow writable signals in two-way bindings (angular#54252)
Updates the TCB generation logic to allow for `WritableSignal` to be assigned in two-way bindings. PR Close angular#54252
1 parent 8aac3c4 commit 67b977e

10 files changed

Lines changed: 190 additions & 33 deletions

File tree

‎packages/compiler-cli/src/ngtsc/testing/fake_core/index.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -136,6 +136,8 @@ export function signal<T>(initialValue: T): WritableSignal<T> {
136136
return null!;
137137
}
138138

139+
export type ɵConditionallyUnwrapSignal<T> = T extends Signal<unknown>? ReturnType<T>: T;
140+
139141
/**
140142
* -------
141143
* Signal inputs

‎packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts‎

Lines changed: 68 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* found in the LICENSE file at https://angular.io/license
77
*/
88

9-
import {AST, BindingPipe, BindingType, BoundTarget, Call, createCssSelectorFromNode, CssSelector, DYNAMIC_TYPE, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, R3Identifiers, SafeCall, SafePropertyRead, SchemaMetadata, SelectorMatcher, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstForLoopBlockEmpty, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstSwitchBlockCase, TmplAstTemplate, TmplAstText, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType} from '@angular/compiler';
9+
import {AST, BindingPipe, BindingType, BoundTarget, Call, createCssSelectorFromNode, CssSelector, DYNAMIC_TYPE, ExpressionType, ImplicitReceiver, ParsedEventType, ParseSourceSpan, PropertyRead, PropertyWrite, R3Identifiers, SafeCall, SafePropertyRead, SchemaMetadata, SelectorMatcher, ThisReceiver, TmplAstBoundAttribute, TmplAstBoundEvent, TmplAstBoundText, TmplAstDeferredBlock, TmplAstDeferredBlockTriggers, TmplAstElement, TmplAstForLoopBlock, TmplAstForLoopBlockEmpty, TmplAstHoverDeferredTrigger, TmplAstIcu, TmplAstIfBlock, TmplAstIfBlockBranch, TmplAstInteractionDeferredTrigger, TmplAstNode, TmplAstReference, TmplAstSwitchBlock, TmplAstSwitchBlockCase, TmplAstTemplate, TmplAstText, TmplAstTextAttribute, TmplAstVariable, TmplAstViewportDeferredTrigger, TransplantedType, TypeofExpr, WrappedNodeExpr} from '@angular/compiler';
1010
import ts from 'typescript';
1111

1212
import {Reference} from '../../imports';
@@ -711,7 +711,7 @@ class TcbDirectiveInputsOp extends TcbOp {
711711

712712
let assignment: ts.Expression = wrapForDiagnostics(expr);
713713

714-
for (const {fieldName, required, transformType, isSignal} of attr.inputs) {
714+
for (const {fieldName, required, transformType, isSignal, isTwoWayBinding} of attr.inputs) {
715715
let target: ts.LeftHandSideExpression;
716716

717717
if (required) {
@@ -791,12 +791,14 @@ class TcbDirectiveInputsOp extends TcbOp {
791791
dirId, ts.factory.createIdentifier(fieldName));
792792
}
793793

794-
// For signal inputs, we unwrap the target `InputSignal`. Note that
795-
// we intentionally do the following things:
796-
// 1. keep the direct access to `dir.[field]` so that modifiers are honored.
797-
// 2. follow the existing pattern where multiple targets assign a single expression.
798-
// This is a significant requirement for language service auto-completion.
799-
if (isSignal) {
794+
if (isTwoWayBinding) {
795+
target = this.getTwoWayBindingExpression(target);
796+
} else if (isSignal) {
797+
// For signal inputs, we unwrap the target `InputSignal`. Note that
798+
// we intentionally do the following things:
799+
// 1. keep the direct access to `dir.[field]` so that modifiers are honored.
800+
// 2. follow the existing pattern where multiple targets assign a single expression.
801+
// This is a significant requirement for language service auto-completion.
800802
const inputSignalBrandWriteSymbol = this.tcb.env.referenceExternalSymbol(
801803
R3Identifiers.InputSignalBrandWriteType.moduleName,
802804
R3Identifiers.InputSignalBrandWriteType.name);
@@ -846,6 +848,52 @@ class TcbDirectiveInputsOp extends TcbOp {
846848
this.tcb.id, this.node, this.dir.name, this.dir.isComponent, missing);
847849
}
848850
}
851+
852+
private getTwoWayBindingExpression(target: ts.LeftHandSideExpression): ts.LeftHandSideExpression {
853+
// TODO(crisbeto): we should be able to avoid the extra variable that captures the type.
854+
// Skipping it for since we don't have a good way to convert the `PropertyAccessExpression`
855+
// into an `QualifiedName`.
856+
// Two-way bindings to inputs allow both the input's defined type and a `WritableSignal`
857+
// of that type. For example `[(value)]="val"` where `@Input() value: number | string`
858+
// allows `val` to be `number | string | WritableSignal<number | string>`. We generate the
859+
// following expressions to expand the type:
860+
// ```
861+
// var captureType = dir.value;
862+
// (id as unknown as ɵConditionallyUnwrapSignal<typeof captureType> |
863+
// WritableSignal<ɵConditionallyUnwrapSignal<typeof captureType>>) = expression;
864+
// ```
865+
// Note that the TCB can be simplified a bit by making the union type part of the utility type
866+
// (e.g. `type ɵTwoWayAssign<T> = T extends Signal ? ReturnType<T> |
867+
// WritableSignal<ReturnType<T>> : ReturnType<T> | WritableSignal<ReturnType<T>>`), however at
868+
// the time of writing, this generates a suboptimal diagnostic message where TS splits up the
869+
// signature, e.g. "Type 'number' is not assignable to type 'string | boolean |
870+
// WritableSignal<string> | WritableSignal<false> | WritableSignal<true>'" instead of Type
871+
// 'number' is not assignable to type 'string | boolean | WritableSignal<string | boolean>'.
872+
const captureType = this.tcb.allocateId();
873+
874+
// ɵConditionallyUnwrapSignal<typeof captureType>
875+
const unwrappedRef = this.tcb.env.referenceExternalType(
876+
R3Identifiers.ConditionallyUnwrapSignal.moduleName,
877+
R3Identifiers.ConditionallyUnwrapSignal.name,
878+
[new ExpressionType(new TypeofExpr(new WrappedNodeExpr(captureType)))]);
879+
880+
// WritableSignal<ɵConditionallyUnwrapSignal<typeof captureType>>
881+
const writableSignalRef = this.tcb.env.referenceExternalType(
882+
R3Identifiers.WritableSignal.moduleName, R3Identifiers.WritableSignal.name,
883+
[new ExpressionType(new WrappedNodeExpr(unwrappedRef))]);
884+
885+
// ɵConditionallyUnwrapSignal<typeof captureType> |
886+
// WritableSignal<ɵConditionallyUnwrapSignal<typeof captureType>>
887+
const type = ts.factory.createUnionTypeNode([unwrappedRef, writableSignalRef]);
888+
this.scope.addStatement(tsCreateVariable(captureType, target));
889+
890+
// (target as unknown as ɵConditionallyUnwrapSignal<typeof captureType> |
891+
// WritableSignal<ɵConditionallyUnwrapSignal<typeof captureType>>)
892+
return ts.factory.createParenthesizedExpression(ts.factory.createAsExpression(
893+
ts.factory.createAsExpression(
894+
target, ts.factory.createKeywordTypeNode(ts.SyntaxKind.UnknownKeyword)),
895+
type));
896+
}
849897
}
850898

851899
/**
@@ -2292,7 +2340,8 @@ interface TcbBoundAttribute {
22922340
fieldName: ClassPropertyName,
22932341
required: boolean,
22942342
isSignal: boolean,
2295-
transformType: Reference<ts.TypeNode>|null
2343+
transformType: Reference<ts.TypeNode>|null,
2344+
isTwoWayBinding: boolean,
22962345
}[];
22972346
}
22982347

@@ -2527,12 +2576,16 @@ function getBoundAttributes(
25272576
if (inputs !== null) {
25282577
boundInputs.push({
25292578
attribute: attr,
2530-
inputs: inputs.map(input => ({
2531-
fieldName: input.classPropertyName,
2532-
required: input.required,
2533-
transformType: input.transform?.type || null,
2534-
isSignal: input.isSignal,
2535-
}))
2579+
inputs: inputs.map(input => {
2580+
return ({
2581+
fieldName: input.classPropertyName,
2582+
required: input.required,
2583+
transformType: input.transform?.type || null,
2584+
isSignal: input.isSignal,
2585+
isTwoWayBinding:
2586+
attr instanceof TmplAstBoundAttribute && attr.type === BindingType.TwoWay,
2587+
});
2588+
})
25362589
});
25372590
}
25382591
};

‎packages/compiler-cli/src/ngtsc/typecheck/test/output_function_diagnostics.spec.ts‎

Lines changed: 0 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -30,24 +30,6 @@ runInEachFileSystem(() => {
3030
`TestComponent.html(1, 24): Property 'x' does not exist on type 'void'.`,
3131
],
3232
},
33-
{
34-
id: 'two way data binding, invalid',
35-
inputs: {'value': {type: 'InputSignal<string>', isSignal: true}},
36-
outputs: {'valueChange': {type: 'OutputEmitter<string>'}},
37-
template: `<div dir [(value)]="bla">`,
38-
component: `bla = true;`,
39-
expected: [
40-
`TestComponent.html(1, 12): Type 'boolean' is not assignable to type 'string'.`,
41-
],
42-
},
43-
{
44-
id: 'two way data binding, valid',
45-
inputs: {'value': {type: 'InputSignal<string>', isSignal: true}},
46-
outputs: {'valueChange': {type: 'OutputEmitter<string>'}},
47-
template: `<div dir [(value)]="bla">`,
48-
component: `bla: string = ''`,
49-
expected: [],
50-
},
5133
{
5234
id: 'complex output object',
5335
outputs: {'evt': {type: 'OutputEmitter<{works: boolean}>'}},

‎packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -648,6 +648,81 @@ describe('type check blocks', () => {
648648
expect(block).toContain('((((this).foo)).$any(((this).a)))');
649649
});
650650

651+
it('should handle a two-way binding to an input/output pair', () => {
652+
const TEMPLATE = `<div twoWay [(input)]="value"></div>`;
653+
const DIRECTIVES: TestDeclaration[] = [{
654+
type: 'directive',
655+
name: 'TwoWay',
656+
selector: '[twoWay]',
657+
inputs: {input: 'input'},
658+
outputs: {inputChange: 'inputChange'},
659+
}];
660+
const block = tcb(TEMPLATE, DIRECTIVES);
661+
expect(block).toContain('var _t1 = null! as i0.TwoWay;');
662+
expect(block).toContain('var _t2 = _t1.input;');
663+
expect(block).toContain(
664+
'(_t1.input as unknown as i1.ɵConditionallyUnwrapSignal<typeof _t2> | ' +
665+
'i1.WritableSignal<i1.ɵConditionallyUnwrapSignal<typeof _t2>>) = (((this).value));');
666+
});
667+
668+
it('should handle a two-way binding to a model()', () => {
669+
const TEMPLATE = `<div twoWay [(input)]="value"></div>`;
670+
const DIRECTIVES: TestDeclaration[] = [{
671+
type: 'directive',
672+
name: 'TwoWay',
673+
selector: '[twoWay]',
674+
inputs: {
675+
input: {
676+
classPropertyName: 'input',
677+
bindingPropertyName: 'input',
678+
required: false,
679+
isSignal: true,
680+
transform: null,
681+
}
682+
},
683+
outputs: {inputChange: 'inputChange'},
684+
}];
685+
const block = tcb(TEMPLATE, DIRECTIVES);
686+
expect(block).toContain('var _t1 = null! as i0.TwoWay;');
687+
expect(block).toContain('var _t2 = _t1.input;');
688+
expect(block).toContain(
689+
'(_t1.input as unknown as i1.ɵConditionallyUnwrapSignal<typeof _t2> | ' +
690+
'i1.WritableSignal<i1.ɵConditionallyUnwrapSignal<typeof _t2>>) = (((this).value));');
691+
});
692+
693+
it('should handle a two-way binding to an input with a transform', () => {
694+
const TEMPLATE = `<div twoWay [(input)]="value"></div>`;
695+
const DIRECTIVES: TestDeclaration[] = [{
696+
type: 'directive',
697+
name: 'TwoWay',
698+
selector: '[twoWay]',
699+
inputs: {
700+
input: {
701+
classPropertyName: 'input',
702+
bindingPropertyName: 'input',
703+
required: false,
704+
isSignal: false,
705+
transform: {
706+
node: ts.factory.createFunctionDeclaration(
707+
undefined, undefined, undefined, undefined, [], undefined, undefined),
708+
type: new Reference(ts.factory.createUnionTypeNode([
709+
ts.factory.createKeywordTypeNode(ts.SyntaxKind.BooleanKeyword),
710+
ts.factory.createKeywordTypeNode(ts.SyntaxKind.StringKeyword),
711+
]))
712+
},
713+
}
714+
},
715+
outputs: {inputChange: 'inputChange'},
716+
coercedInputFields: ['input'],
717+
}];
718+
const block = tcb(TEMPLATE, DIRECTIVES);
719+
expect(block).toContain('var _t1 = null! as boolean | string;');
720+
expect(block).toContain('var _t2 = _t1;');
721+
expect(block).toContain(
722+
'(_t1 as unknown as i1.ɵConditionallyUnwrapSignal<typeof _t2> | ' +
723+
'i1.WritableSignal<i1.ɵConditionallyUnwrapSignal<typeof _t2>>) = (((this).value));');
724+
});
725+
651726
describe('experimental DOM checking via lib.dom.d.ts', () => {
652727
it('should translate unclaimed bindings to their property equivalent', () => {
653728
const TEMPLATE = `<label [for]="'test'"></label>`;

‎packages/compiler-cli/src/ngtsc/typecheck/testing/index.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,8 @@ export function angularCoreDts(): TestFile {
122122
asReadonly(): Signal<T>;
123123
}
124124
125+
export type ɵConditionallyUnwrapSignal<T> = T extends Signal<unknown>? ReturnType<T>: T;
126+
125127
/**
126128
* -------
127129
* Signal inputs

‎packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2355,6 +2355,35 @@ export declare class AnimationEvent {
23552355
`Type 'HTMLDivElement' is not assignable to type ` +
23562356
`'HTMLInputElement | ElementRef<HTMLInputElement>'`);
23572357
});
2358+
2359+
it('should type check a two-way binding to an input with a transform', () => {
2360+
env.tsconfig({strictTemplates: true});
2361+
env.write('test.ts', `
2362+
import {Component, Directive, Input, Output, EventEmitter} from '@angular/core';
2363+
2364+
export function toNumber(val: boolean | string) { return 1; }
2365+
2366+
@Directive({selector: '[dir]', standalone: true})
2367+
export class CoercionDir {
2368+
@Input({transform: toNumber}) val!: number;
2369+
@Output() valChange = new EventEmitter<number>();
2370+
}
2371+
2372+
@Component({
2373+
template: '<input dir [(val)]="invalidType">',
2374+
standalone: true,
2375+
imports: [CoercionDir],
2376+
})
2377+
export class FooCmp {
2378+
invalidType = 1;
2379+
}
2380+
`);
2381+
const diags = env.driveDiagnostics();
2382+
expect(diags.length).toBe(1);
2383+
expect(diags[0].messageText)
2384+
.toBe(
2385+
`Type 'number' is not assignable to type 'string | boolean | WritableSignal<string | boolean>'.`);
2386+
});
23582387
});
23592388

23602389
describe('restricted inputs', () => {

‎packages/compiler/src/render3/r3_identifiers.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,4 +408,6 @@ export class Identifiers {
408408
// type-checking
409409
static InputSignalBrandWriteType = {name: 'ɵINPUT_SIGNAL_BRAND_WRITE_TYPE', moduleName: CORE};
410410
static UnwrapDirectiveSignalInputs = {name: 'ɵUnwrapDirectiveSignalInputs', moduleName: CORE};
411+
static WritableSignal = {name: 'WritableSignal', moduleName: CORE};
412+
static ConditionallyUnwrapSignal = {name: 'ɵConditionallyUnwrapSignal', moduleName: CORE};
411413
}

‎packages/core/src/core_reactivity_export_internal.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ export {
2020
CreateSignalOptions,
2121
signal,
2222
WritableSignal,
23+
ɵConditionallyUnwrapSignal,
2324
} from './render3/reactivity/signal';
2425
export {
2526
untracked,

‎packages/core/src/render3/reactivity/signal.ts‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,15 @@ export interface CreateSignalOptions<T> {
4343
equal?: ValueEqualityFn<T>;
4444
}
4545

46+
/**
47+
* Type used during template type checking to either unwrap a signal or preserve a value if it's
48+
* not a signal. E.g. `ɵConditionallyUnwrapSignal<Signal<number>>` yields `number` while
49+
* `ɵConditionallyUnwrapSignal<number>` preserves the `number` type.
50+
*
51+
* @codeGenApi
52+
*/
53+
export type ɵConditionallyUnwrapSignal<T> = T extends Signal<unknown>? ReturnType<T>: T;
54+
4655
/**
4756
* Create a `Signal` that can be set or updated directly.
4857
*/

‎packages/core/test/render3/jit_environment_spec.ts‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,8 @@ const AOT_ONLY = new Set<string>([
3434
// used in type-checking.
3535
'ɵINPUT_SIGNAL_BRAND_WRITE_TYPE',
3636
'ɵUnwrapDirectiveSignalInputs',
37+
'ɵConditionallyUnwrapSignal',
38+
'WritableSignal',
3739
]);
3840

3941
/**

0 commit comments

Comments
 (0)