refactor(migrations): always add readonly to migrated signal inputs (#57368)

devversion · AndrewKushnir · commit bbc970bb0b31 · 2024-08-13T09:41:09.000-07:00
Signal inputs are no longer updated by assignment, unlike `@Input()`, so a good practice is adding `readonly` for the `InputSignal`— which should never be swapped out. This is a safe operation because the migration skips all inputs that are being written anyway. 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
@@ -32,6 +32,7 @@ export function convertToSignalInput(
     resolvedType,
     preferShorthandIfPossible,
     isUndefinedInitialValue,
+    originalInputDecorator,
   }: ConvertInputPreparation,
   checker: ts.TypeChecker,
   importManager: ImportManager,
@@ -117,12 +118,16 @@ export function convertToSignalInput(
     inputArgs,
   );
 
-  // TODO:
-  //   - modifiers (but private does not work)
-  //   - preserve custom decorators etc.
+  let modifiersWithoutInputDecorator =
+    node.modifiers?.filter((m) => m !== originalInputDecorator.node) ?? [];
+
+  // Add `readonly` to all new signal input declarations.
+  if (!modifiersWithoutInputDecorator?.some((s) => s.kind === ts.SyntaxKind.ReadonlyKeyword)) {
+    modifiersWithoutInputDecorator.push(ts.factory.createModifier(ts.SyntaxKind.ReadonlyKeyword));
+  }
 
   const result = ts.factory.createPropertyDeclaration(
-    undefined,
+    modifiersWithoutInputDecorator,
     node.name,
     undefined,
     undefined,
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
@@ -13,6 +13,8 @@ import {
   InputMemberIncompatibility,
 } from '../input_detection/incompatibility';
 import {InputNode} from '../input_detection/input_node';
+import {Decorator} from '../../../../../../compiler-cli/src/ngtsc/reflection';
+import assert from 'assert';
 
 /**
  * Interface describing analysis performed when the input
@@ -23,6 +25,7 @@ export interface ConvertInputPreparation {
   preferShorthandIfPossible: {originalType: ts.TypeNode} | null;
   isUndefinedInitialValue: boolean;
   resolvedMetadata: ExtractedInput;
+  originalInputDecorator: Decorator;
 }
 
 /**
@@ -46,6 +49,12 @@ export function prepareAndCheckForConversion(
       reason: InputIncompatibilityReason.Accessor,
     };
   }
+
+  assert(
+    metadata.inputDecorator !== null,
+    'Expected an input decorator for inputs that are being migrated.',
+  );
+
   const initialValue = node.initializer;
 
   // If an input can be required, due to the non-null assertion on the property,
@@ -118,5 +127,6 @@ export function prepareAndCheckForConversion(
     resolvedType: typeToAdd,
     preferShorthandIfPossible,
     isUndefinedInitialValue,
+    originalInputDecorator: metadata.inputDecorator,
   };
 }
diff --git a/packages/core/schematics/migrations/signal-migration/src/input_detection/input_decorator.ts b/packages/core/schematics/migrations/signal-migration/src/input_detection/input_decorator.ts
@@ -24,7 +24,7 @@ import {
 } from '../../../../../../compiler-cli/src/ngtsc/partial_evaluator';
 import {
   ClassDeclaration,
-  DecoratorIdentifier,
+  Decorator,
   ReflectionHost,
 } from '../../../../../../compiler-cli/src/ngtsc/reflection';
 import {CompilationMode} from '../../../../../../compiler-cli/src/ngtsc/transform';
@@ -34,6 +34,7 @@ import {InputNode, isInputContainerNode} from '../input_detection/input_node';
 /** Metadata extracted of an input declaration (in `.ts` or `.d.ts` files). */
 export interface ExtractedInput extends InputMapping {
   inSourceFile: boolean;
+  inputDecorator: Decorator | null;
 }
 
 /** Attempts to extract metadata of a potential TypeScript `@Input()` declaration. */
@@ -86,6 +87,7 @@ function extractDtsInput(node: ts.Node, metadataReader: DtsMetadataReader): Extr
     ? null
     : {
         ...inputMapping,
+        inputDecorator: null,
         inSourceFile: false,
       };
 }
@@ -150,6 +152,7 @@ function extractSourceCodeInput(
     isSignal: false,
     inSourceFile: true,
     transform: transformResult,
+    inputDecorator,
   };
 }
 
diff --git a/packages/core/schematics/migrations/signal-migration/test/golden-test/modifier_tests.ts b/packages/core/schematics/migrations/signal-migration/test/golden-test/modifier_tests.ts
@@ -0,0 +1,15 @@
+// tslint:disable
+
+import {Component, Input} from '@angular/core';
+
+function CustomDecorator() {
+  return (a: any, b: any) => {};
+}
+
+@Component({template: ''})
+class ModifierScenarios {
+  @Input() readonly alreadyReadonly = true;
+  @Input() protected ImProtected = true;
+  @Input() protected readonly ImProtectedAndReadonly = true;
+  @Input() @CustomDecorator() protected readonly usingCustomDecorator = true;
+}
diff --git a/packages/core/schematics/migrations/signal-migration/test/golden.txt b/packages/core/schematics/migrations/signal-migration/test/golden.txt
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
