Skip to content

Commit a17f6cb

Browse files
crisbetothePunderWoman
crisbeto
authored and
thePunderWoman
committed
refactor(compiler-cli): rework TCB for two-way bindings (angular#54252)
Reworks the TCB for two-way bindings to make them simpler and to avoid regressions for two-way bindings to generic inputs. The new TCB looks as follows: ``` var _t1: Dir; var _t2 = _t1.input; (_t1 as typeof _t2 | WritableSignal<typeof _t2>) = expression; ``` PR Close angular#54252
1 parent 372e1ff commit a17f6cb

11 files changed

Lines changed: 202 additions & 63 deletions

File tree

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

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -136,8 +136,6 @@ 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-
141139
/**
142140
* -------
143141
* Signal inputs

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

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -358,17 +358,16 @@ export class SymbolBuilder {
358358
for (const node of nodes) {
359359
let assignment: ts.PropertyAccessExpression|ts.ElementAccessExpression|null = null;
360360

361-
// One-way bindings usually are in the form of `dir.input = expression`.
362361
if (isAccessExpression(node.left)) {
362+
// One-way bindings usually are in the form of `dir.input = expression`.
363363
assignment = node.left;
364364
} else if (
365-
// The property side of two-way bindings is in the
366-
// form of `(dir.input as unknown as someType) = expression`.
367365
isTwoWayBinding && ts.isParenthesizedExpression(node.left) &&
368366
ts.isAsExpression(node.left.expression) &&
369-
ts.isAsExpression(node.left.expression.expression) &&
370-
isAccessExpression(node.left.expression.expression.expression)) {
371-
assignment = node.left.expression.expression.expression;
367+
isAccessExpression(node.left.expression.expression)) {
368+
// The property side of two-way bindings is in the
369+
// form of `(dir.input as someType) = expression`.
370+
assignment = node.left.expression.expression;
372371
}
373372

374373
if (assignment === null) {

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

Lines changed: 19 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -791,9 +791,7 @@ class TcbDirectiveInputsOp extends TcbOp {
791791
dirId, ts.factory.createIdentifier(fieldName));
792792
}
793793

794-
if (isTwoWayBinding) {
795-
target = this.getTwoWayBindingExpression(target);
796-
} else if (isSignal) {
794+
if (isSignal) {
797795
// For signal inputs, we unwrap the target `InputSignal`. Note that
798796
// we intentionally do the following things:
799797
// 1. keep the direct access to `dir.[field]` so that modifiers are honored.
@@ -811,6 +809,10 @@ class TcbDirectiveInputsOp extends TcbOp {
811809
target = ts.factory.createElementAccessExpression(target, inputSignalBrandWriteSymbol);
812810
}
813811

812+
if (isTwoWayBinding) {
813+
target = this.getTwoWayBindingTarget(target);
814+
}
815+
814816
if (attr.attribute.keySpan !== undefined) {
815817
addParseSpanInfo(target, attr.attribute.keySpan);
816818
}
@@ -849,50 +851,33 @@ class TcbDirectiveInputsOp extends TcbOp {
849851
}
850852
}
851853

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`.
854+
private getTwoWayBindingTarget(target: ts.LeftHandSideExpression): ts.LeftHandSideExpression {
856855
// Two-way bindings to inputs allow both the input's defined type and a `WritableSignal`
857856
// of that type. For example `[(value)]="val"` where `@Input() value: number | string`
858857
// allows `val` to be `number | string | WritableSignal<number | string>`. We generate the
859858
// following expressions to expand the type:
860859
// ```
861860
// var captureType = dir.value;
862-
// (id as unknown as ɵConditionallyUnwrapSignal<typeof captureType> |
863-
// WritableSignal<ɵConditionallyUnwrapSignal<typeof captureType>>) = expression;
861+
// (dir.value as typeof captureType | WritableSignal<typeof captureType>) = expression;
864862
// ```
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>'.
863+
// Some notes:
864+
// - We wrap the assignment, insted of for example declaring a variable and assigning to it,
865+
// because keeping the assignment makes it easier to do lookups in the language service.
866+
// - The `captureType` variable can be inline, but for signal input expressions it can be
867+
// long so we use it make the code a bit neater. It also saves us some code that would
868+
// have to convert property/element access expressions into type query nodes.
872869
const captureType = this.tcb.allocateId();
870+
const captureTypeVar = tsCreateVariable(captureType, target);
871+
markIgnoreDiagnostics(captureTypeVar);
872+
this.scope.addStatement(captureTypeVar);
873873

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>>
874+
const typeQuery = ts.factory.createTypeQueryNode(captureType);
881875
const writableSignalRef = this.tcb.env.referenceExternalType(
882876
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));
877+
[new ExpressionType(new TypeofExpr(new WrappedNodeExpr(captureType)))]);
889878

890-
// (target as unknown as ɵConditionallyUnwrapSignal<typeof captureType> |
891-
// WritableSignal<ɵConditionallyUnwrapSignal<typeof captureType>>)
892879
return ts.factory.createParenthesizedExpression(ts.factory.createAsExpression(
893-
ts.factory.createAsExpression(
894-
target, ts.factory.createKeywordTypeNode(ts.SyntaxKind.UnknownKeyword)),
895-
type));
880+
target, ts.factory.createUnionTypeNode([typeQuery, writableSignalRef])));
896881
}
897882
}
898883

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

Lines changed: 4 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -661,8 +661,7 @@ describe('type check blocks', () => {
661661
expect(block).toContain('var _t1 = null! as i0.TwoWay;');
662662
expect(block).toContain('var _t2 = _t1.input;');
663663
expect(block).toContain(
664-
'(_t1.input as unknown as i1.ɵConditionallyUnwrapSignal<typeof _t2> | ' +
665-
'i1.WritableSignal<i1.ɵConditionallyUnwrapSignal<typeof _t2>>) = (((this).value));');
664+
'(_t1.input as typeof _t2 | i1.WritableSignal<typeof _t2>) = (((this).value));');
666665
});
667666

668667
it('should handle a two-way binding to a model()', () => {
@@ -684,10 +683,9 @@ describe('type check blocks', () => {
684683
}];
685684
const block = tcb(TEMPLATE, DIRECTIVES);
686685
expect(block).toContain('var _t1 = null! as i0.TwoWay;');
687-
expect(block).toContain('var _t2 = _t1.input;');
686+
expect(block).toContain('var _t2 = _t1.input[i1.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE];');
688687
expect(block).toContain(
689-
'(_t1.input as unknown as i1.ɵConditionallyUnwrapSignal<typeof _t2> | ' +
690-
'i1.WritableSignal<i1.ɵConditionallyUnwrapSignal<typeof _t2>>) = (((this).value));');
688+
'(_t1.input[i1.ɵINPUT_SIGNAL_BRAND_WRITE_TYPE] as typeof _t2 | i1.WritableSignal<typeof _t2>) = (((this).value));');
691689
});
692690

693691
it('should handle a two-way binding to an input with a transform', () => {
@@ -719,8 +717,7 @@ describe('type check blocks', () => {
719717
expect(block).toContain('var _t1 = null! as boolean | string;');
720718
expect(block).toContain('var _t2 = _t1;');
721719
expect(block).toContain(
722-
'(_t1 as unknown as i1.ɵConditionallyUnwrapSignal<typeof _t2> | ' +
723-
'i1.WritableSignal<i1.ɵConditionallyUnwrapSignal<typeof _t2>>) = (((this).value));');
720+
'(_t1 as typeof _t2 | i1.WritableSignal<typeof _t2>) = (((this).value));');
724721
});
725722

726723
describe('experimental DOM checking via lib.dom.d.ts', () => {

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

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

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

Lines changed: 145 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -586,6 +586,37 @@ runInEachFileSystem(() => {
586586
}));
587587
});
588588

589+
it('should check two-way binding of a signal to a decorator-based input/output pair', () => {
590+
env.write('test.ts', `
591+
import {Component, Directive, Input, Output, signal, EventEmitter} from '@angular/core';
592+
593+
@Directive({
594+
selector: '[dir]',
595+
standalone: true,
596+
})
597+
export class TestDir {
598+
@Input() value = 0;
599+
@Output() valueChange = new EventEmitter<number>();
600+
}
601+
602+
@Component({
603+
standalone: true,
604+
template: \`<div dir [(value)]="value"></div>\`,
605+
imports: [TestDir],
606+
})
607+
export class TestComp {
608+
value = signal(false);
609+
}
610+
`);
611+
612+
const diags = env.driveDiagnostics();
613+
expect(diags.length).toBe(1);
614+
expect(diags[0].messageText).toEqual(jasmine.objectContaining({
615+
messageText:
616+
`Type 'WritableSignal<boolean>' is not assignable to type 'number | WritableSignal<number>'.`
617+
}));
618+
});
619+
589620
it('should not allow a non-writable signal to be assigned to a model', () => {
590621
env.write('test.ts', `
591622
import {Component, Directive, model, input} from '@angular/core';
@@ -696,6 +727,120 @@ runInEachFileSystem(() => {
696727
expect(diagnostics[0].messageText)
697728
.toBe(`Required input 'value' from directive TestDir must be specified.`);
698729
});
730+
731+
it('should check generic two-way model binding with a primitive value', () => {
732+
env.write('test.ts', `
733+
import {Component, Directive, model} from '@angular/core';
734+
735+
@Directive({
736+
selector: '[dir]',
737+
standalone: true,
738+
})
739+
export class TestDir<T extends {id: string}> {
740+
value = model.required<T>();
741+
}
742+
743+
@Component({
744+
standalone: true,
745+
template: \`<div dir [(value)]="value"></div>\`,
746+
imports: [TestDir],
747+
})
748+
export class TestComp {
749+
value = {id: 1};
750+
}
751+
`);
752+
753+
const diags = env.driveDiagnostics();
754+
expect(diags.length).toBe(1);
755+
expect(diags[0].messageText).toEqual(jasmine.objectContaining({
756+
messageText:
757+
`Type '{ id: number; }' is not assignable to type '{ id: string; } | WritableSignal<{ id: string; }>'.`
758+
}));
759+
});
760+
761+
it('should check generic two-way model binding with a signal value', () => {
762+
env.write('test.ts', `
763+
import {Component, Directive, model, signal} from '@angular/core';
764+
765+
@Directive({
766+
selector: '[dir]',
767+
standalone: true,
768+
})
769+
export class TestDir<T extends {id: string}> {
770+
value = model.required<T>();
771+
}
772+
773+
@Component({
774+
standalone: true,
775+
template: \`<div dir [(value)]="value"></div>\`,
776+
imports: [TestDir],
777+
})
778+
export class TestComp {
779+
value = signal({id: 1});
780+
}
781+
`);
782+
783+
const diags = env.driveDiagnostics();
784+
expect(diags.length).toBe(1);
785+
expect(diags[0].messageText).toEqual(jasmine.objectContaining({
786+
messageText:
787+
`Type 'WritableSignal<{ id: number; }>' is not assignable to type '{ id: string; } | WritableSignal<{ id: string; }>'.`
788+
}));
789+
});
790+
791+
it('should report unwrapped signals assigned to a model in a one-way binding', () => {
792+
env.write('test.ts', `
793+
import {Component, Directive, model, signal} from '@angular/core';
794+
795+
@Directive({
796+
selector: '[dir]',
797+
standalone: true,
798+
})
799+
export class TestDir {
800+
value = model(0);
801+
}
802+
803+
@Component({
804+
standalone: true,
805+
template: \`<div dir [value]="value"></div>\`,
806+
imports: [TestDir],
807+
})
808+
export class TestComp {
809+
value = signal(1);
810+
}
811+
`);
812+
813+
const diagnostics = env.driveDiagnostics();
814+
expect(diagnostics.length).toBe(1);
815+
expect(diagnostics[0].messageText)
816+
.toBe(`Type 'WritableSignal<number>' is not assignable to type 'number'.`);
817+
});
818+
});
819+
820+
it('should allow two-way binding to a generic model input', () => {
821+
env.write('test.ts', `
822+
import {Component, Directive, model, signal} from '@angular/core';
823+
824+
@Directive({
825+
selector: '[dir]',
826+
standalone: true,
827+
})
828+
export class TestDir<T> {
829+
value = model.required<T>();
830+
}
831+
832+
@Component({
833+
standalone: true,
834+
template: \`<div dir [(value)]="value"></div>\`,
835+
imports: [TestDir],
836+
})
837+
export class TestComp {
838+
value = signal(1);
839+
}
840+
`);
841+
842+
const diags = env.driveDiagnostics();
843+
expect(diags).toEqual([]);
699844
});
700845
});
701846
});

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

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -418,6 +418,35 @@ export declare class AnimationEvent {
418418
expect(getSourceCodeForDiagnostic(diags[0].relatedInformation![1])).toBe('ChildCmpDir');
419419
});
420420

421+
it('should type check a two-way binding to a generic property', () => {
422+
env.tsconfig({strictTemplates: true});
423+
env.write('test.ts', `
424+
import {Component, Directive, Input, Output, EventEmitter} from '@angular/core';
425+
426+
@Directive({selector: '[dir]', standalone: true})
427+
export class Dir<T extends {id: string}> {
428+
@Input() val!: T;
429+
@Output() valChange = new EventEmitter<T>();
430+
}
431+
432+
@Component({
433+
template: '<input dir [(val)]="invalidType">',
434+
standalone: true,
435+
imports: [Dir],
436+
})
437+
export class FooCmp {
438+
invalidType = {id: 1};
439+
}
440+
`);
441+
442+
const diags = env.driveDiagnostics();
443+
expect(diags.length).toBe(1);
444+
expect(diags[0].messageText).toEqual(jasmine.objectContaining({
445+
messageText:
446+
`Type '{ id: number; }' is not assignable to type '{ id: string; } | WritableSignal<{ id: string; }>'.`
447+
}));
448+
});
449+
421450
describe('strictInputTypes', () => {
422451
beforeEach(() => {
423452
env.write('test.ts', `

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

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -409,5 +409,4 @@ export class Identifiers {
409409
static InputSignalBrandWriteType = {name: 'ɵINPUT_SIGNAL_BRAND_WRITE_TYPE', moduleName: CORE};
410410
static UnwrapDirectiveSignalInputs = {name: 'ɵUnwrapDirectiveSignalInputs', moduleName: CORE};
411411
static WritableSignal = {name: 'WritableSignal', moduleName: CORE};
412-
static ConditionallyUnwrapSignal = {name: 'ɵConditionallyUnwrapSignal', moduleName: CORE};
413412
}

‎packages/core/src/core_reactivity_export_internal.ts‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,6 @@ export {
2020
CreateSignalOptions,
2121
signal,
2222
WritableSignal,
23-
ɵConditionallyUnwrapSignal,
2423
} from './render3/reactivity/signal';
2524
export {
2625
untracked,

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

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -43,15 +43,6 @@ 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-
5546
/**
5647
* Create a `Signal` that can be set or updated directly.
5748
*/

0 commit comments

Comments
 (0)