refactor(migrations): use input() shorthand if possible in input migration (#57368)

devversion · AndrewKushnir · commit 87d00d26ff06 · 2024-08-13T09:41:08.000-07:00
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
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
@@ -9,23 +9,30 @@
 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.
  *
  * @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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -374,7 +374,7 @@ export class AppComponent {
   input = input<string | null>(null);
   bla = input.required<boolean, string | boolean>({ transform: disabledTransform });
   narrowableMultipleTimes = input<Vehicle | null>(null);
-  withUndefinedInput = input<string | undefined>(undefined);
+  withUndefinedInput = input<string>();
   @Input() incompatible: string | null = null;
 
   private _bla: any;
@@ -720,7 +720,7 @@ import { Directive, input } from '@angular/core';
 
 @Directive()
 class OptionalInput {
-  bla = input<string | undefined>(undefined);
+  bla = input<string>();
 }
 @@@@@@ problematic_type_reference.ts @@@@@@
 
@@ -1065,5 +1065,5 @@ class WithJsdoc {
    */
   simpleInput = input.required<string>();
 
-  withCommentInside = input</* intended */ boolean | undefined>(undefined);
+  withCommentInside = input</* intended */ boolean>();
 }
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
@@ -374,7 +374,7 @@ export class AppComponent {
   input = input<string | null>(null);
   bla = input.required<boolean, string | boolean>({ transform: disabledTransform });
   narrowableMultipleTimes = input<Vehicle | null>(null);
-  withUndefinedInput = input<string | undefined>(undefined);
+  withUndefinedInput = input<string>();
   incompatible = input<string | null>(null);
 
   private _bla: any;
@@ -720,7 +720,7 @@ import { Directive, input } from '@angular/core';
 
 @Directive()
 class OptionalInput {
-  bla = input<string | undefined>(undefined);
+  bla = input<string>();
 }
 @@@@@@ problematic_type_reference.ts @@@@@@
 
@@ -1065,5 +1065,5 @@ class WithJsdoc {
    */
   simpleInput = input.required<string>();
 
-  withCommentInside = input</* intended */ boolean | undefined>(undefined);
+  withCommentInside = input</* intended */ boolean>();
 }
