From 1b4eaea6d4fa00a722eaed011c904e5cca048501 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 (#33243) 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 PR Close #33243 --- .../src/ngtsc/metadata/src/api.ts | 1 + .../src/ngtsc/metadata/src/util.ts | 24 ++++++- .../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 | 27 +++++++ .../test/ngtsc/template_typecheck_spec.ts | 71 +++++++++++++++++++ 8 files changed, 142 insertions(+), 5 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..70b662f868ae1 100644 --- a/packages/compiler-cli/src/ngtsc/metadata/src/util.ts +++ b/packages/compiler-cli/src/ngtsc/metadata/src/util.ts @@ -82,20 +82,25 @@ 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 { if (!member.name.startsWith('ngTemplateGuard_')) { return null; } - const inputName = member.name.split('_', 2)[1]; + const inputName = afterUnderscore(member.name); if (member.kind === ClassMemberKind.Property) { let type: string|null = null; if (member.type !== null && ts.isLiteralTypeNode(member.type) && @@ -115,6 +120,13 @@ function extractTemplateGuard(member: ClassMember): TemplateGuardMeta|null { } } +function extractCoercedInput(member: ClassMember): string|null { + if (!member.name.startsWith('ngCoerceInput_')) { + return null !; + } + return afterUnderscore(member.name); +} + /** * A `MetadataReader` that reads from an ordered set of child readers until it obtains the requested * metadata. @@ -158,3 +170,11 @@ export class CompoundMetadataReader implements MetadataReader { return null; } } + +function afterUnderscore(str: string): string { + const pos = str.indexOf('_'); + if (pos === -1) { + throw new Error(`Expected '${str}' to contain '_'`); + } + return str.substr(pos + 1); +} 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 4c9138d4a36ab..d78f18638e08e 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 49484e9a20fc3..2584aa63380f5 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 @@ -1144,6 +1144,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 9d443eb2bd71f..534858ccd641a 100644 --- a/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts +++ b/packages/compiler-cli/src/ngtsc/typecheck/test/test_utils.ts @@ -160,9 +160,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, @@ -295,6 +300,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 030395fb4168b..7a1a092c7d5b9 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 @@ -268,7 +268,34 @@ describe('type check blocks', () => { expect(block).toContain( '_t1.addEventListener("event", $event => (ctx).foo(($event as any)));'); }); + }); + + 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', () => { diff --git a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts index 2791395b1b405..871e62815de5c 100644 --- a/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts +++ b/packages/compiler-cli/test/ngtsc/template_typecheck_spec.ts @@ -406,6 +406,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}); });