fix(migrations): preserve optional parameters (#57367)

crisbeto · AndrewKushnir · commit 5d76401ff5e0 · 2024-08-13T09:54:25.000-07:00
Makes it so the inject migration preserves the optional token when declaring a parameter. This came up in some testing as something that can be potentially breaking for classes that implement interfaces. PR Close #57367
diff --git a/packages/core/schematics/ng-generate/inject-migration/migration.ts b/packages/core/schematics/ng-generate/inject-migration/migration.ts
@@ -321,12 +321,17 @@ function migrateParameter(
   // If the parameter declares a property, we need to declare it (e.g. `private foo: Foo`).
   if (declaresProp) {
     const prop = ts.factory.createPropertyDeclaration(
-      node.modifiers?.filter((modifier) => {
-        // Strip out the DI decorators, as well as `public` which is redundant.
-        return !ts.isDecorator(modifier) && modifier.kind !== ts.SyntaxKind.PublicKeyword;
-      }),
+      cloneModifiers(
+        node.modifiers?.filter((modifier) => {
+          // Strip out the DI decorators, as well as `public` which is redundant.
+          return !ts.isDecorator(modifier) && modifier.kind !== ts.SyntaxKind.PublicKeyword;
+        }),
+      ),
       name,
-      undefined,
+      // Don't add the question token to private properties since it won't affect interface implementation.
+      node.modifiers?.some((modifier) => modifier.kind === ts.SyntaxKind.PrivateKeyword)
+        ? undefined
+        : node.questionToken,
       // We can't initialize the property if it's referenced within a `super` call.
       // See the logic further below for the initialization.
       usedInSuper ? node.type : undefined,
@@ -610,3 +615,15 @@ function replaceNodePlaceholder(
   const result = printer.printNode(ts.EmitHint.Unspecified, node, sourceFile);
   return result.replace(PLACEHOLDER, replacement);
 }
+
+/**
+ * Clones an optional array of modifiers. Can be useful to
+ * strip the comments from a node with modifiers.
+ */
+function cloneModifiers(modifiers: ts.ModifierLike[] | ts.NodeArray<ts.ModifierLike> | undefined) {
+  return modifiers?.map((modifier) => {
+    return ts.isDecorator(modifier)
+      ? ts.factory.createDecorator(modifier.expression)
+      : ts.factory.createModifier(modifier.kind);
+  });
+}
diff --git a/packages/core/schematics/test/inject_migration_spec.ts b/packages/core/schematics/test/inject_migration_spec.ts
@@ -1341,6 +1341,60 @@ describe('inject migration', () => {
     ]);
   });
 
+  it('should mark optional members if they correspond to optional parameters', async () => {
+    writeFile(
+      '/dir.ts',
+      [
+        `import { Directive, Optional } from '@angular/core';`,
+        `import { Foo } from 'foo';`,
+        ``,
+        `@Directive()`,
+        `class MyDir {`,
+        `  constructor(@Optional() public foo?: Foo) {}`,
+        `}`,
+      ].join('\n'),
+    );
+
+    await runMigration();
+
+    expect(tree.readContent('/dir.ts').split('\n')).toEqual([
+      `import { Directive, inject } from '@angular/core';`,
+      `import { Foo } from 'foo';`,
+      ``,
+      `@Directive()`,
+      `class MyDir {`,
+      `  foo? = inject(Foo, { optional: true });`,
+      `}`,
+    ]);
+  });
+
+  it('should not mark private members as optional', async () => {
+    writeFile(
+      '/dir.ts',
+      [
+        `import { Directive, Optional } from '@angular/core';`,
+        `import { Foo } from 'foo';`,
+        ``,
+        `@Directive()`,
+        `class MyDir {`,
+        `  constructor(@Optional() private foo?: Foo) {}`,
+        `}`,
+      ].join('\n'),
+    );
+
+    await runMigration();
+
+    expect(tree.readContent('/dir.ts').split('\n')).toEqual([
+      `import { Directive, inject } from '@angular/core';`,
+      `import { Foo } from 'foo';`,
+      ``,
+      `@Directive()`,
+      `class MyDir {`,
+      `  private foo = inject(Foo, { optional: true });`,
+      `}`,
+    ]);
+  });
+
   describe('internal-only behavior', () => {
     function runInternalMigration() {
       return runMigration({_internalCombineMemberInitializers: true});
