fix(migrations): inject migration aggressively removing imports (#58959)

crisbeto · pkozlowski-opensource · commit a4924af6d580 · 2024-12-02T08:56:21.000+01:00
Fixes that the inject migration would remove imports even if they're still used by classes that aren't being migrated. PR Close #58959
diff --git a/packages/core/schematics/ng-generate/inject-migration/analysis.ts b/packages/core/schematics/ng-generate/inject-migration/analysis.ts
@@ -9,6 +9,7 @@
 import ts from 'typescript';
 import {getAngularDecorators} from '../../utils/ng_decorators';
 import {getNamedImports} from '../../utils/typescript/imports';
+import {closestNode} from '../../utils/typescript/nodes';
 
 /** Options that can be used to configure the migration. */
 export interface MigrationOptions {
@@ -99,12 +100,27 @@ export function analyzeFile(
       return;
     }
 
-    // Only visit the initializer of parameters, because we won't exclude
-    // their decorators from the identifier counting result below.
     if (ts.isParameter(node)) {
+      const closestConstructor = closestNode(node, ts.isConstructorDeclaration);
+
+      // Visiting the same parameters that we're about to remove can throw off the reference
+      // counting logic below. If we run into an initializer, we always visit its initializer
+      // and optionally visit the modifiers/decorators if it's not due to be deleted. Note that
+      // here we technically aren't dealing with the the full list of classes, but the parent class
+      // will have been visited by the time we reach the parameters.
       if (node.initializer) {
         walk(node.initializer);
       }
+
+      if (
+        closestConstructor === null ||
+        // This is meant to avoid the case where this is a
+        // parameter inside a function placed in a constructor.
+        !closestConstructor.parameters.includes(node) ||
+        !classes.some((c) => c.constructor === closestConstructor)
+      ) {
+        node.modifiers?.forEach(walk);
+      }
       return;
     }
 
diff --git a/packages/core/schematics/test/inject_migration_spec.ts b/packages/core/schematics/test/inject_migration_spec.ts
@@ -1684,6 +1684,45 @@ describe('inject migration', () => {
     ]);
   });
 
+  it('should not remove decorator imports if unmigrated classes are still using them', async () => {
+    writeFile(
+      '/dir.ts',
+      [
+        `import { Directive, Optional } from '@angular/core';`,
+        `import { Foo } from 'foo';`,
+        `import { Bar } from 'bar';`,
+        ``,
+        `@Directive()`,
+        `class WillMigrate {`,
+        `  constructor(@Optional() private foo: Foo) {}`,
+        `}`,
+        ``,
+        `@Directive()`,
+        `abstract class WillNotMigrate {`,
+        `  constructor(@Optional() private bar: Bar) {}`,
+        `}`,
+      ].join('\n'),
+    );
+
+    await runMigration();
+
+    expect(tree.readContent('/dir.ts').split('\n')).toEqual([
+      `import { Directive, Optional, inject } from '@angular/core';`,
+      `import { Foo } from 'foo';`,
+      `import { Bar } from 'bar';`,
+      ``,
+      `@Directive()`,
+      `class WillMigrate {`,
+      `  private foo = inject(Foo, { optional: true });`,
+      `}`,
+      ``,
+      `@Directive()`,
+      `abstract class WillNotMigrate {`,
+      `  constructor(@Optional() private bar: Bar) {}`,
+      `}`,
+    ]);
+  });
+
   describe('internal-only behavior', () => {
     function runInternalMigration() {
       return runMigration({_internalCombineMemberInitializers: true});
