From fea1589d0ccee4345a7e143bb0564119bc83dd0f Mon Sep 17 00:00:00 2001 From: Alex Rickabaugh Date: Thu, 17 Oct 2019 16:02:21 -0700 Subject: [PATCH] feat(ivy): input type coercion for template type-checking Often the types of an `@Input`'s field don't fully reflect the types of assignable values. This can happen when an input has a getter/setter pair where the getter always returns a narrow type, and the setter coerces a wider value down to the narrow type. For example, you could imagine an input of the form: ```typescript @Input() get value(): string { return this._value; } set value(v: {toString(): string}) { this._value = v.toString(); } ``` Here, the getter always returns a `string`, but the setter accepts any value that can be `toString()`'d, and coerces it to a string. Unfortunately TypeScript does not actually support this syntax, and so Angular users are forced to type their setters as narrowly as the getters, even though at runtime the coercion works just fine. To support these kinds of patterns (e.g. as used by Material), this commit adds a compiler feature called "input coercion". When a binding is made to the 'value' input of a directive like MatInput, the compiler will look for a static function with the name ngCoerceInput_value. If such a function is found, the type-checking expression for the input will be wrapped in a call to the function, allowing for the expression of a type conversion between the binding expression and the value being written to the input's field. To solve the case above, for example, MatInput might write: ```typescript class MatInput { // rest of the directive... static ngCoerceInput_value(value: {toString(): string}): string { return null!; } } ``` FW-1475 #resolve --- .../src/ngtsc/metadata/src/api.ts | 1 + .../src/ngtsc/metadata/src/util.ts | 14 +++- .../src/ngtsc/scope/test/local_spec.ts | 1 + .../src/ngtsc/typecheck/src/api.ts | 1 + .../ngtsc/typecheck/src/type_check_block.ts | 10 +++ .../src/ngtsc/typecheck/test/test_utils.ts | 12 +++- .../typecheck/test/type_check_block_spec.ts | 28 ++++++++ .../test/ngtsc/template_typecheck_spec.ts | 71 +++++++++++++++++++ 8 files changed, 134 insertions(+), 4 deletions(-) diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts index 34b4202c8144c..04dab5219990e 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/api.ts @@ -34,6 +34,7 @@ export interface DirectiveMeta extends T2DirectiveMeta { queries: string[]; ngTemplateGuards: TemplateGuardMeta[]; hasNgTemplateContextGuard: boolean; + coercedInputs: Set; /** * A `Reference` to the base class for the directive, if one was detected. diff --git a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts index 7f8a9d2678fdb..56f5f9b7ed140 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts @@ -82,13 +82,18 @@ export function readStringArrayType(type: ts.TypeNode): string[] { export function extractDirectiveGuards(node: ClassDeclaration, reflector: ReflectionHost): { ngTemplateGuards: TemplateGuardMeta[], hasNgTemplateContextGuard: boolean, + coercedInputs: Set, } { const staticMembers = reflector.getMembersOfClass(node).filter(member => member.isStatic); const ngTemplateGuards = staticMembers.map(extractTemplateGuard) .filter((guard): guard is TemplateGuardMeta => guard !== null); const hasNgTemplateContextGuard = staticMembers.some( member => member.kind === ClassMemberKind.Method && member.name === 'ngTemplateContextGuard'); - return {hasNgTemplateContextGuard, ngTemplateGuards}; + + const coercedInputs = + new Set(staticMembers.map(extractCoercedInput) + .filter((inputName): inputName is string => inputName !== null)); + return {hasNgTemplateContextGuard, ngTemplateGuards, coercedInputs}; } function extractTemplateGuard(member: ClassMember): TemplateGuardMeta|null { @@ -115,6 +120,13 @@ function extractTemplateGuard(member: ClassMember): TemplateGuardMeta|null { } } +function extractCoercedInput(member: ClassMember): string|null { + if (!member.name.startsWith('ngCoerceInput_')) { + return null !; + } + return member.name.split('_', 2)[1]; +} + /** * A `MetadataReader` that reads from an ordered set of child readers until it obtains the requested * metadata. diff --git a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts index 008f17199b03d..ad6813d7d0b38 100644 --- a/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts +++ b/packages/compiler-cli/src/ngtsc/scope/test/local_spec.ts @@ -228,6 +228,7 @@ function fakeDirective(ref: Reference): DirectiveMeta { queries: [], hasNgTemplateContextGuard: false, ngTemplateGuards: [], + coercedInputs: new Set(), baseClass: null, }; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts index 9653d492c14cc..232ddb2617cae 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/api.ts @@ -21,6 +21,7 @@ export interface TypeCheckableDirectiveMeta extends DirectiveMeta { ref: Reference; queries: string[]; ngTemplateGuards: TemplateGuardMeta[]; + coercedInputs: Set; hasNgTemplateContextGuard: boolean; } diff --git a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts index ed37665032201..fce88adb54955 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/src/type_check_block.ts @@ -880,6 +880,16 @@ function tcbGetDirectiveInputs( expr = ts.createStringLiteral(attr.value); } + // Wrap the expression if the directive has a coercion function provided. + if (dir.coercedInputs.has(attr.name)) { + const dirId = tcb.env.reference(dir.ref as Reference>); + const coercionFn = ts.createPropertyAccess(dirId, `ngCoerceInput_${attr.name}`); + expr = ts.createCall( + /* expression */ coercionFn, + /* typeArguments */ undefined, + /* argumentsArray */[expr]); + } + directiveInputs.push({ type: 'binding', field: field, diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts index d3ce287d7c849..373fffe1f96df 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -128,9 +128,14 @@ export const ALL_ENABLED_CONFIG: TypeCheckingConfig = { }; // Remove 'ref' from TypeCheckableDirectiveMeta and add a 'selector' instead. -export type TestDirective = - Partial>>& - {selector: string, name: string, file?: AbsoluteFsPath, type: 'directive'}; +export type TestDirective = Partial>>& + { + selector: string, + name: string, file?: AbsoluteFsPath, + type: 'directive', coercedInputs?: string[], + }; export type TestPipe = { name: string, file?: AbsoluteFsPath, @@ -257,6 +262,7 @@ function prepareDeclarations( inputs: decl.inputs || {}, isComponent: decl.isComponent || false, ngTemplateGuards: decl.ngTemplateGuards || [], + coercedInputs: new Set(decl.coercedInputs || []), outputs: decl.outputs || {}, queries: decl.queries || [], }; diff --git a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts index d2bac46b00765..8ab9f19be3ba2 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/type_check_block_spec.ts @@ -235,6 +235,34 @@ describe('type check blocks', () => { }); }); + describe('input coercion', () => { + it('should coerce a basic input', () => { + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'MatInput', + selector: '[matInput]', + inputs: {'value': 'value'}, + coercedInputs: ['value'], + }]; + const TEMPLATE = ``; + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain('value: (MatInput.ngCoerceInput_value((ctx).expr))'); + }); + + it('should coerce based on input name, not field name', () => { + const DIRECTIVES: TestDeclaration[] = [{ + type: 'directive', + name: 'MatInput', + selector: '[matInput]', + inputs: {'field': 'value'}, + coercedInputs: ['value'], + }]; + const TEMPLATE = ``; + const block = tcb(TEMPLATE, DIRECTIVES); + expect(block).toContain('field: (MatInput.ngCoerceInput_value((ctx).expr))'); + }); + }); + describe('config', () => { const DIRECTIVES: TestDeclaration[] = [{ type: 'directive', diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index f6b5dd4da94ac..0305135140dc2 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -370,6 +370,77 @@ export declare class CommonModule { expect(diags[1].length).toEqual(15); }); + describe('input coercion', () => { + beforeEach(() => { + env.tsconfig({ + 'fullTemplateTypeCheck': true, + }); + env.write('node_modules/@angular/material/index.d.ts', ` + import * as i0 from '@angular/core'; + + export declare class MatInput { + value: string; + static ɵdir: i0.ɵɵDirectiveDefWithMeta; + static ngCoerceInput_value(v: string|number): string; + } + + export declare class MatInputModule { + static ɵmod: i0.ɵɵNgModuleDefWithMeta; + } + `); + }); + + it('should coerce an input using a coercion function if provided', () => { + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + import {MatInputModule} from '@angular/material'; + + @Component({ + selector: 'blah', + template: '', + }) + export class FooCmp { + someNumber = 3; + } + + @NgModule({ + declarations: [FooCmp], + imports: [MatInputModule], + }) + export class FooModule {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(0); + }); + + it('should give an error if the binding expression type is not accepted by the coercion function', + () => { + env.write('test.ts', ` + import {Component, NgModule} from '@angular/core'; + import {MatInputModule} from '@angular/material'; + + @Component({ + selector: 'blah', + template: '', + }) + export class FooCmp { + invalidType = true; + } + + @NgModule({ + declarations: [FooCmp], + imports: [MatInputModule], + }) + export class FooModule {} + `); + const diags = env.driveDiagnostics(); + expect(diags.length).toBe(1); + expect(diags[0].messageText) + .toBe( + `Argument of type 'boolean' is not assignable to parameter of type 'string | number'.`); + }); + }); + describe('legacy schema checking with the DOM schema', () => { beforeEach( () => { env.tsconfig({ivyTemplateTypeCheck: true, fullTemplateTypeCheck: false}); });