fix(migrations): don't migrate classes with parameters that can't be injected (#58959)

crisbeto · pkozlowski-opensource · commit 7191aa6e09ca · 2024-12-02T08:56:21.000+01:00
Updates the inject migrations to skip over classes that have uninjectable class parameters. 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
@@ -62,6 +62,16 @@ export const DI_PARAM_SYMBOLS = new Set([
   'forwardRef',
 ]);
 
+/** Kinds of nodes which aren't injectable when set as a type of a parameter. */
+const UNINJECTABLE_TYPE_KINDS = new Set([
+  ts.SyntaxKind.TrueKeyword,
+  ts.SyntaxKind.FalseKeyword,
+  ts.SyntaxKind.NumberKeyword,
+  ts.SyntaxKind.StringKeyword,
+  ts.SyntaxKind.NullKeyword,
+  ts.SyntaxKind.VoidKeyword,
+]);
+
 /**
  * Finds the necessary information for the `inject` migration in a file.
  * @param sourceFile File which to analyze.
@@ -156,9 +166,26 @@ export function analyzeFile(
           member.parameters.length > 0,
       ) as ts.ConstructorDeclaration | undefined;
 
+      // Basic check to determine if all parameters are injectable. This isn't exhaustive, but it
+      // should catch the majority of cases. An exhaustive check would require a full type checker
+      // which we don't have in this migration.
+      const allParamsInjectable = !!constructorNode?.parameters.every((param) => {
+        if (!param.type || !UNINJECTABLE_TYPE_KINDS.has(param.type.kind)) {
+          return true;
+        }
+        return getAngularDecorators(localTypeChecker, ts.getDecorators(param) || []).some(
+          (dec) => dec.name === 'Inject' || dec.name === 'Attribute',
+        );
+      });
+
       // Don't migrate abstract classes by default, because
       // their parameters aren't guaranteed to be injectable.
-      if (supportsDI && constructorNode && (!isAbstract || options.migrateAbstractClasses)) {
+      if (
+        supportsDI &&
+        constructorNode &&
+        allParamsInjectable &&
+        (!isAbstract || options.migrateAbstractClasses)
+      ) {
         classes.push({
           node,
           constructor: constructorNode,
diff --git a/packages/core/schematics/test/inject_migration_spec.ts b/packages/core/schematics/test/inject_migration_spec.ts
@@ -1341,6 +1341,24 @@ describe('inject migration', () => {
     ]);
   });
 
+  it('should not migrate class that has un-injectable parameters', async () => {
+    const initialText = [
+      `import { Directive, Inject } from '@angular/core';`,
+      `import { FOO_TOKEN, Foo } from 'foo';`,
+      ``,
+      `@Directive()`,
+      `class MyDir {`,
+      `  constructor(readonly injectable: Foo, private notInjectable: string) {}`,
+      `}`,
+    ].join('\n');
+
+    writeFile('/dir.ts', initialText);
+
+    await runMigration();
+
+    expect(tree.readContent('/dir.ts')).toBe(initialText);
+  });
+
   it('should unwrap forwardRef with an implicit return', async () => {
     writeFile(
       '/dir.ts',
