From 00a79d0ee2ab358db4f83bd2644f4345fdd62a92 Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Tue, 1 Oct 2024 13:46:43 +0000 Subject: [PATCH] refactor(migrations): properly migrate inputs marked as optional via question mark (#58031) Currently if inputs are marked as optional via the question mark syntax, we add `undefined` only if there is an explicit type. This is wrong as we should do the same if there is just an initializer. This commit fixes this. PR Close #58031 --- .../src/convert-input/prepare_and_check.ts | 104 +++++++++++++----- .../src/input_detection/incompatibility.ts | 9 +- .../input_detection/incompatibility_human.ts | 7 ++ .../test/golden-test/optional_inputs.ts | 1 + .../signal-migration/test/golden.txt | 1 + .../test/golden_best_effort.txt | 1 + 6 files changed, 91 insertions(+), 32 deletions(-) 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 244d90631b0..e242d7f1abd 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 @@ -91,15 +91,33 @@ export function prepareAndCheckForConversion( // 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)) - ) { - typeToAdd = ts.factory.createUnionTypeNode([ - node.type, - ts.factory.createKeywordTypeNode(ts.SyntaxKind.UndefinedKeyword), - ]); + if (node.questionToken !== undefined) { + // If there is no type, but we have an initial value, try inferring + // it from the initializer. + if (typeToAdd === undefined && initialValue !== undefined) { + const inferredType = inferImportableTypeForInput(checker, node, initialValue); + if (inferredType !== null) { + typeToAdd = inferredType; + } + } + if (typeToAdd === undefined) { + return { + context: node, + reason: InputIncompatibilityReason.InputWithQuestionMarkButNoGoodExplicitTypeExtractable, + }; + } + + if ( + !checker.isTypeAssignableTo( + checker.getUndefinedType(), + checker.getTypeFromTypeNode(typeToAdd), + ) + ) { + typeToAdd = ts.factory.createUnionTypeNode([ + typeToAdd, + ts.factory.createKeywordTypeNode(ts.SyntaxKind.UndefinedKeyword), + ]); + } } let leadingTodoText: string | null = null; @@ -128,25 +146,9 @@ 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) { - const propertyType = checker.getTypeAtLocation(node); - if (propertyType.flags & ts.TypeFlags.Boolean) { - typeToAdd = ts.factory.createKeywordTypeNode(ts.SyntaxKind.BooleanKeyword); - } else if (propertyType.flags & ts.TypeFlags.String) { - typeToAdd = ts.factory.createKeywordTypeNode(ts.SyntaxKind.StringKeyword); - } else if (propertyType.flags & ts.TypeFlags.Number) { - typeToAdd = ts.factory.createKeywordTypeNode(ts.SyntaxKind.NumberKeyword); - } else if (ts.isIdentifier(initialValue)) { - // @Input({required: true}) bla = SOME_DEFAULT; - typeToAdd = ts.factory.createTypeQueryNode(initialValue); - } else if ( - ts.isPropertyAccessExpression(initialValue) && - ts.isIdentifier(initialValue.name) && - ts.isIdentifier(initialValue.expression) - ) { - // @Input({required: true}) bla = prop.SOME_DEFAULT; - typeToAdd = ts.factory.createTypeQueryNode( - ts.factory.createQualifiedName(initialValue.name, initialValue.expression), - ); + const inferredType = inferImportableTypeForInput(checker, node, initialValue); + if (inferredType !== null) { + typeToAdd = inferredType; } else { // Note that we could use `typeToTypeNode` here but it's likely breaking because // the generated type might depend on imports that we cannot add here (nor want). @@ -167,3 +169,49 @@ export function prepareAndCheckForConversion( leadingTodoText, }; } + +function inferImportableTypeForInput( + checker: ts.TypeChecker, + node: InputNode, + initialValue: ts.Node, +): ts.TypeNode | null { + const propertyType = checker.getTypeAtLocation(node); + + // If the resolved type is a primitive, or union of primitive types, + // return a type node fully derived from the resolved type. + if ( + isPrimitiveImportableTypeNode(propertyType) || + (propertyType.isUnion() && propertyType.types.every(isPrimitiveImportableTypeNode)) + ) { + return checker.typeToTypeNode(propertyType, node, ts.NodeBuilderFlags.NoTypeReduction) ?? null; + } + + // Alternatively, try to infer a simple importable type from\ + // the initializer. + + if (ts.isIdentifier(initialValue)) { + // @Input({required: true}) bla = SOME_DEFAULT; + return ts.factory.createTypeQueryNode(initialValue); + } else if ( + ts.isPropertyAccessExpression(initialValue) && + ts.isIdentifier(initialValue.name) && + ts.isIdentifier(initialValue.expression) + ) { + // @Input({required: true}) bla = prop.SOME_DEFAULT; + return ts.factory.createTypeQueryNode( + ts.factory.createQualifiedName(initialValue.name, initialValue.expression), + ); + } + + return null; +} + +function isPrimitiveImportableTypeNode(type: ts.Type): boolean { + return !!( + type.flags & ts.TypeFlags.BooleanLike || + type.flags & ts.TypeFlags.StringLike || + type.flags & ts.TypeFlags.NumberLike || + type.flags & ts.TypeFlags.Undefined || + type.flags & ts.TypeFlags.Null + ); +} diff --git a/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility.ts b/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility.ts index 1e0f2587212..61764da4a83 100644 --- a/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility.ts +++ b/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility.ts @@ -22,10 +22,11 @@ export enum InputIncompatibilityReason { SpyOnThatOverwritesField = 5, PotentiallyNarrowedInTemplateButNoSupportYet = 6, RequiredInputButNoGoodExplicitTypeExtractable = 7, - WriteAssignment = 8, - Accessor = 9, - OutsideOfMigrationScope = 10, - SkippedViaConfigFilter = 11, + InputWithQuestionMarkButNoGoodExplicitTypeExtractable = 8, + WriteAssignment = 9, + Accessor = 10, + OutsideOfMigrationScope = 11, + SkippedViaConfigFilter = 12, } /** Reasons why a whole class and its inputs cannot be migrated. */ diff --git a/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human.ts b/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human.ts index d98213e8ae3..794f636b169 100644 --- a/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human.ts +++ b/packages/core/schematics/migrations/signal-migration/src/input_detection/incompatibility_human.ts @@ -53,6 +53,13 @@ export function getMessageForInputIncompatibility(reason: InputIncompatibilityRe short: `Input is required, but the migration cannot determine a good type for the input.`, extra: 'Consider adding an explicit type to make the migration possible.', }; + case InputIncompatibilityReason.InputWithQuestionMarkButNoGoodExplicitTypeExtractable: + return { + short: `Input is marked with a question mark. Migration could not determine a good type for the input.`, + extra: + 'The migration needs to be able to resolve a type, so that it can include `undefined` in your type. ' + + 'Consider adding an explicit type to make the migration possible.', + }; case InputIncompatibilityReason.SkippedViaConfigFilter: return { short: `This input is not part of the current migration scope.`, diff --git a/packages/core/schematics/migrations/signal-migration/test/golden-test/optional_inputs.ts b/packages/core/schematics/migrations/signal-migration/test/golden-test/optional_inputs.ts index 2124e84f42c..a25a938afb7 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden-test/optional_inputs.ts +++ b/packages/core/schematics/migrations/signal-migration/test/golden-test/optional_inputs.ts @@ -5,4 +5,5 @@ import {Directive, Input} from '@angular/core'; @Directive() class OptionalInput { @Input() bla?: string; + @Input() isLegacyHttpOnly? = false; } diff --git a/packages/core/schematics/migrations/signal-migration/test/golden.txt b/packages/core/schematics/migrations/signal-migration/test/golden.txt index 630be44717b..0e9d8e818d0 100644 --- a/packages/core/schematics/migrations/signal-migration/test/golden.txt +++ b/packages/core/schematics/migrations/signal-migration/test/golden.txt @@ -942,6 +942,7 @@ import {Directive, input} from '@angular/core'; @Directive() class OptionalInput { readonly bla = input(); + readonly isLegacyHttpOnly = input(false); } @@@@@@ problematic_type_reference.ts @@@@@@ 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 dd7bf344a0d..4fd4609e2e1 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 @@ -915,6 +915,7 @@ import {Directive, input} from '@angular/core'; @Directive() class OptionalInput { readonly bla = input(); + readonly isLegacyHttpOnly = input(false); } @@@@@@ problematic_type_reference.ts @@@@@@