From 87d00d26ff06073dbb9d15c93bb746bc58ddb06e Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 13 Aug 2024 11:00:30 +0000 Subject: [PATCH] refactor(migrations): use `input()` shorthand if possible in input migration (#57368) In some cases, the migration can detect when `input()` as a shorthand may be usable. This commit adds such detection and migrates inputs to this form when possible. PR Close #57368 --- .../src/convert-input/convert_to_signal.ts | 59 +++++++++++-------- .../src/convert-input/prepare_and_check.ts | 28 +++++---- .../passes/6_migrate_input_declarations.ts | 4 +- .../signal-migration/src/phase_migrate.ts | 2 +- .../src/utils/remove_from_union.ts | 20 +++++++ .../signal-migration/test/golden.txt | 6 +- .../test/golden_best_effort.txt | 6 +- 7 files changed, 82 insertions(+), 43 deletions(-) create mode 100644 packages/core/schematics/migrations/signal-migration/src/utils/remove_from_union.ts diff --git a/packages/core/schematics/migrations/signal-migration/src/convert-input/convert_to_signal.ts b/packages/core/schematics/migrations/signal-migration/src/convert-input/convert_to_signal.ts index e0f619aefd5..c298b06d3f1 100644 --- a/packages/core/schematics/migrations/signal-migration/src/convert-input/convert_to_signal.ts +++ b/packages/core/schematics/migrations/signal-migration/src/convert-input/convert_to_signal.ts @@ -9,13 +9,16 @@ import assert from 'assert'; import ts from 'typescript'; -import {MigrationHost} from '../migration_host'; import {ConvertInputPreparation} from './prepare_and_check'; import {DecoratorInputTransform} from '../../../../../../compiler-cli/src/ngtsc/metadata'; import {ImportManager} from '../../../../../../compiler-cli/src/ngtsc/translator'; +import {removeFromUnionIfPossible} from '../utils/remove_from_union'; const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed}); +// TODO: Consider initializations inside the constructor. Those are not migrated right now +// though, as they are writes. + /** * * Converts an `@Input()` property declaration to a signal input. @@ -23,9 +26,13 @@ const printer = ts.createPrinter({newLine: ts.NewLineKind.LineFeed}); * @returns The transformed property declaration, printed as a string. */ export function convertToSignalInput( - host: MigrationHost, node: ts.PropertyDeclaration, - {resolvedMetadata: metadata, resolvedType, isResolvedTypeCheckable}: ConvertInputPreparation, + { + resolvedMetadata: metadata, + resolvedType, + preferShorthandIfPossible, + isUndefinedInitialValue, + }: ConvertInputPreparation, checker: ts.TypeChecker, importManager: ImportManager, ): string { @@ -46,16 +53,33 @@ export function convertToSignalInput( ); } if (metadata.transform !== null) { - properties.push( - extractTransformOfInput(metadata.transform, resolvedType, isResolvedTypeCheckable, checker), - ); + properties.push(extractTransformOfInput(metadata.transform, resolvedType, checker)); } optionsLiteral = ts.factory.createObjectLiteralExpression(properties); } - const strictPropertyInitialization = - !!host.options.strict || !!host.options.strictPropertyInitialization; + // The initial value is `undefined` or none is present: + // - We may be able to use the `input()` shorthand + // - or we use an explicit `undefined` initial value. + if (isUndefinedInitialValue) { + // Shorthand not possible, so explicitly add `undefined`. + if (preferShorthandIfPossible === null) { + initialValue = ts.factory.createIdentifier('undefined'); + } else { + resolvedType = preferShorthandIfPossible.originalType; + + // When using the `input()` shorthand, try cutting of `undefined` from potential + // union types. `undefined` will be automatically included in the type. + if (ts.isUnionTypeNode(resolvedType)) { + resolvedType = removeFromUnionIfPossible( + resolvedType, + (t) => t.kind !== ts.SyntaxKind.UndefinedKeyword, + ); + } + } + } + const inputArgs: ts.Expression[] = []; const typeArguments: ts.TypeNode[] = []; @@ -67,19 +91,6 @@ export function convertToSignalInput( } } - // If we have no initial value but strict property initialization is enabled, we - // need to add an explicit value. Alternatively, if we have an explicit type, we - // need to add an explicit initial value as per the API signature of `input()`. - if (initialValue === undefined && (strictPropertyInitialization || resolvedType !== undefined)) { - // TODO: Consider initializations inside the constructor. Those are not migrated right now - // though, as they are writes. - - // TODO: We can use the `input()` shorthand if there is a question mark? - // We can assume `undefined` is part of the type already, either already was included, or - // we added synthetically as part of the preparation. - initialValue = ts.factory.createIdentifier('undefined'); - } - // Always add an initial value when the input is optional, and we have one, or we need one // to be able to pass options as the second argument. if (!metadata.required && (initialValue !== undefined || optionsLiteral !== null)) { @@ -128,7 +139,6 @@ export function convertToSignalInput( function extractTransformOfInput( transform: DecoratorInputTransform, resolvedType: ts.TypeNode | undefined, - isResolvedTypeCheckable: boolean, checker: ts.TypeChecker, ): ts.PropertyAssignment { assert(ts.isExpression(transform.node), `Expected transform to be an expression.`); @@ -138,7 +148,10 @@ function extractTransformOfInput( // In some cases, the transform function is not compatible because with decorator inputs, // those were not checked. We cast the transform to `any` and add a TODO. // TODO: Insert a TODO and capture this in the design doc. - if (resolvedType !== undefined && isResolvedTypeCheckable) { + if (resolvedType !== undefined && !ts.isSyntheticExpression(resolvedType)) { + // Note: If the type is synthetic, we cannot check, and we accept that in the worst case + // we will create code that is not necessarily compiling. This is unlikely, but notably + // the errors would be correct and valuable. const transformType = checker.getTypeAtLocation(transform.node); const transformSignature = transformType.getCallSignatures()[0]; assert(transformSignature !== undefined, 'Expected transform to be an invoke-able.'); diff --git a/packages/core/schematics/migrations/signal-migration/src/convert-input/prepare_and_check.ts b/packages/core/schematics/migrations/signal-migration/src/convert-input/prepare_and_check.ts index a2a3cbc792c..cdc5fc1aa5d 100644 --- a/packages/core/schematics/migrations/signal-migration/src/convert-input/prepare_and_check.ts +++ b/packages/core/schematics/migrations/signal-migration/src/convert-input/prepare_and_check.ts @@ -20,7 +20,8 @@ import {InputNode} from '../input_detection/input_node'; */ export interface ConvertInputPreparation { resolvedType: ts.TypeNode | undefined; - isResolvedTypeCheckable: boolean; + preferShorthandIfPossible: {originalType: ts.TypeNode} | null; + isUndefinedInitialValue: boolean; resolvedMetadata: ExtractedInput; } @@ -53,18 +54,27 @@ export function prepareAndCheckForConversion( metadata.required = true; } + const isUndefinedInitialValue = + node.initializer === undefined || + (ts.isIdentifier(node.initializer) && node.initializer.text === 'undefined'); let typeToAdd: ts.TypeNode | undefined = node.type; - let isResolvedTypeCheckable = true; + let preferShorthandIfPossible: {originalType: ts.TypeNode} | null = null; - // If the input was using `@Input() bla?: string;`, then we try to explicitly - // add `undefined` as type, if it's not part of the type already. + // If there is no initial value, or it's `undefined`, we can prefer the `input()` + // shorthand which automatically uses `undefined` as initial value, and includes it + // in the input type. + if (!metadata.required && node.type !== undefined && isUndefinedInitialValue) { + preferShorthandIfPossible = {originalType: node.type}; + } + + // If the input is using `@Input() bla?: string;` with the "optional question mark", + // then we try to explicitly add `undefined` as type, if it's not part of the type already. + // This is ensuring correctness, as `bla?` automatically includes `undefined` currently. if ( node.type !== undefined && node.questionToken !== undefined && !checker.isTypeAssignableTo(checker.getUndefinedType(), checker.getTypeFromTypeNode(node.type)) ) { - // Synthetic types are never checkable. - isResolvedTypeCheckable = false; typeToAdd = ts.factory.createUnionTypeNode([ node.type, ts.factory.createKeywordTypeNode(ts.SyntaxKind.UndefinedKeyword), @@ -74,9 +84,6 @@ export function prepareAndCheckForConversion( // Attempt to extract type from input initial value. No explicit type, but input is required. // Hence we need an explicit type, or fall back to `typeof`. if (typeToAdd === undefined && initialValue !== undefined && metadata.required) { - // Synthetic types are never checkable. - isResolvedTypeCheckable = false; - const propertyType = checker.getTypeAtLocation(node); if (propertyType.flags & ts.TypeFlags.Boolean) { typeToAdd = ts.factory.createKeywordTypeNode(ts.SyntaxKind.BooleanKeyword); @@ -108,7 +115,8 @@ export function prepareAndCheckForConversion( return { resolvedMetadata: metadata, - isResolvedTypeCheckable, resolvedType: typeToAdd, + preferShorthandIfPossible, + isUndefinedInitialValue, }; } diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/6_migrate_input_declarations.ts b/packages/core/schematics/migrations/signal-migration/src/passes/6_migrate_input_declarations.ts index f37bce93d98..f47478bc584 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/6_migrate_input_declarations.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/6_migrate_input_declarations.ts @@ -11,7 +11,6 @@ import {MigrationResult} from '../result'; import {Replacement} from '../replacement'; import {convertToSignalInput} from '../convert-input/convert_to_signal'; import assert from 'assert'; -import {MigrationHost} from '../migration_host'; import {KnownInputs} from '../input_detection/known_inputs'; import {ImportManager} from '../../../../../../compiler-cli/src/ngtsc/translator'; @@ -20,7 +19,6 @@ import {ImportManager} from '../../../../../../compiler-cli/src/ngtsc/translator * manages imports within the given file. */ export function pass6__migrateInputDeclarations( - host: MigrationHost, checker: ts.TypeChecker, result: MigrationResult, knownInputs: KnownInputs, @@ -46,7 +44,7 @@ export function pass6__migrateInputDeclarations( new Replacement( input.node.getStart(), input.node.getEnd(), - convertToSignalInput(host, input.node, metadata, checker, importManager), + convertToSignalInput(input.node, metadata, checker, importManager), ), ); } diff --git a/packages/core/schematics/migrations/signal-migration/src/phase_migrate.ts b/packages/core/schematics/migrations/signal-migration/src/phase_migrate.ts index 70ca1c8b16f..d448080e3c6 100644 --- a/packages/core/schematics/migrations/signal-migration/src/phase_migrate.ts +++ b/packages/core/schematics/migrations/signal-migration/src/phase_migrate.ts @@ -41,7 +41,7 @@ export function executeMigrationPhase( // Migrate passes. pass5__migrateTypeScriptReferences(result, typeChecker, knownInputs); - pass6__migrateInputDeclarations(host, typeChecker, result, knownInputs, importManager); + pass6__migrateInputDeclarations(typeChecker, result, knownInputs, importManager); pass7__migrateTemplateReferences(host, result, knownInputs); pass8__migrateHostBindings(result, knownInputs); pass9__migrateTypeScriptTypeReferences(result, knownInputs, importManager); diff --git a/packages/core/schematics/migrations/signal-migration/src/utils/remove_from_union.ts b/packages/core/schematics/migrations/signal-migration/src/utils/remove_from_union.ts new file mode 100644 index 00000000000..43c8df19f26 --- /dev/null +++ b/packages/core/schematics/migrations/signal-migration/src/utils/remove_from_union.ts @@ -0,0 +1,20 @@ +/** + * @license + * Copyright Google LLC All Rights Reserved. + * + * Use of this source code is governed by an MIT-style license that can be + * found in the LICENSE file at https://angular.io/license + */ + +import ts from 'typescript'; + +export function removeFromUnionIfPossible( + union: ts.UnionTypeNode, + filter: (v: ts.TypeNode) => boolean, +): ts.UnionTypeNode { + const filtered = union.types.filter(filter); + if (filtered.length === union.types.length) { + return union; + } + return ts.factory.updateUnionTypeNode(union, ts.factory.createNodeArray(filtered)); +} diff --git a/packages/core/schematics/migrations/signal-migration/test/golden.txt b/packages/core/schematics/migrations/signal-migration/test/golden.txt index 381ec49327f..20d556d9e60 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden.txt @@ -374,7 +374,7 @@ export class AppComponent { input = input(null); bla = input.required({ transform: disabledTransform }); narrowableMultipleTimes = input(null); - withUndefinedInput = input(undefined); + withUndefinedInput = input(); @Input() incompatible: string | null = null; private _bla: any; @@ -720,7 +720,7 @@ import { Directive, input } from '@angular/core'; @Directive() class OptionalInput { - bla = input(undefined); + bla = input(); } @@@@@@ problematic_type_reference.ts @@@@@@ @@ -1065,5 +1065,5 @@ class WithJsdoc { */ simpleInput = input.required(); - withCommentInside = input(undefined); + withCommentInside = input(); } diff --git a/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt b/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt index 24ae4f116cf..12d803c9634 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden_best_effort.txt @@ -374,7 +374,7 @@ export class AppComponent { input = input(null); bla = input.required({ transform: disabledTransform }); narrowableMultipleTimes = input(null); - withUndefinedInput = input(undefined); + withUndefinedInput = input(); incompatible = input(null); private _bla: any; @@ -720,7 +720,7 @@ import { Directive, input } from '@angular/core'; @Directive() class OptionalInput { - bla = input(undefined); + bla = input(); } @@@@@@ problematic_type_reference.ts @@@@@@ @@ -1065,5 +1065,5 @@ class WithJsdoc { */ simpleInput = input.required(); - withCommentInside = input(undefined); + withCommentInside = input(); }