refactor(migrations): internal inject migration applying some changes to abstract classes (#58427)

We were filtering out abstract classes pretty late in the migration which led to the internal part of it to make some changes that aren't finalized later. These changes fix the issue by filtering out abstract classes during analysis.

PR Close #58427

packages/core/schematics/ng-generate/inject-migration/analysis.ts

@@ -10,6 +10,37 @@ import ts from 'typescript';
 import {getAngularDecorators} from '../../utils/ng_decorators';
 import {getNamedImports} from '../../utils/typescript/imports';
 
+/** Options that can be used to configure the migration. */
+export interface MigrationOptions {
+  /** Whether to generate code that keeps injectors backwards compatible. */
+  backwardsCompatibleConstructors?: boolean;
+
+  /** Whether to migrate abstract classes. */
+  migrateAbstractClasses?: boolean;
+
+  /** Whether to make the return type of `@Optinal()` parameters to be non-nullable. */
+  nonNullableOptional?: boolean;
+
+  /**
+   * Internal-only option that determines whether the migration should try to move the
+   * initializers of class members from the constructor back into the member itself. E.g.
+   *
+   * ```
+   * // Before
+   * private foo;
+   *
+   * constructor(@Inject(BAR) private bar: Bar) {
+   *   this.foo = this.bar.getValue();
+   * }
+   *
+   * // After
+   * private bar = inject(BAR);
+   * private foo = this.bar.getValue();
+   * ```
+   */
+  _internalCombineMemberInitializers?: boolean;
+}
+
 /** Names of decorators that enable DI on a class declaration. */
 const DECORATORS_SUPPORTING_DI = new Set([
   'Component',
@@ -35,7 +66,11 @@ export const DI_PARAM_SYMBOLS = new Set([
  * @param sourceFile File which to analyze.
  * @param localTypeChecker Type checker scoped to the specific file.
  */
-export function analyzeFile(sourceFile: ts.SourceFile, localTypeChecker: ts.TypeChecker) {
+export function analyzeFile(
+  sourceFile: ts.SourceFile,
+  localTypeChecker: ts.TypeChecker,
+  options: MigrationOptions,
+) {
   const coreSpecifiers = getNamedImports(sourceFile, '@angular/core');
 
   // Exit early if there are no Angular imports.
@@ -96,6 +131,7 @@ export function analyzeFile(sourceFile: ts.SourceFile, localTypeChecker: ts.Type
       }
     } else if (ts.isClassDeclaration(node)) {
       const decorators = getAngularDecorators(localTypeChecker, ts.getDecorators(node) || []);
+      const isAbstract = !!node.modifiers?.some((m) => m.kind === ts.SyntaxKind.AbstractKeyword);
       const supportsDI = decorators.some((dec) => DECORATORS_SUPPORTING_DI.has(dec.name));
       const constructorNode = node.members.find(
         (member) =>
@@ -104,7 +140,9 @@ export function analyzeFile(sourceFile: ts.SourceFile, localTypeChecker: ts.Type
           member.parameters.length > 0,
       ) as ts.ConstructorDeclaration | undefined;
 
-      if (supportsDI && constructorNode) {
+      // Don't migrate abstract classes by default, because
+      // their parameters aren't guaranteed to be injectable.
+      if (supportsDI && constructorNode && (!isAbstract || options.migrateAbstractClasses)) {
         classes.push({
           node,
           constructor: constructorNode,

packages/core/schematics/ng-generate/inject-migration/index.ts

@@ -12,7 +12,8 @@ import {join, relative} from 'path';
 import {normalizePath} from '../../utils/change_tracker';
 import {canMigrateFile, createMigrationProgram} from '../../utils/typescript/compiler_host';
 
-import {migrateFile, MigrationOptions} from './migration';
+import {migrateFile} from './migration';
+import {MigrationOptions} from './analysis';
 
 interface Options extends MigrationOptions {
   path: string;

packages/core/schematics/ng-generate/inject-migration/migration.ts

@@ -16,6 +16,7 @@ import {
   isNullableType,
   parameterDeclaresProperty,
   DI_PARAM_SYMBOLS,
+  MigrationOptions,
 } from './analysis';
 import {getAngularDecorators} from '../../utils/ng_decorators';
 import {getImportOfIdentifier} from '../../utils/typescript/imports';
@@ -29,37 +30,6 @@ import {getLeadingLineWhitespaceOfNode} from '../../utils/tsurge/helpers/ast/lea
  */
 const PLACEHOLDER = 'ɵɵngGeneratePlaceholderɵɵ';
 
-/** Options that can be used to configure the migration. */
-export interface MigrationOptions {
-  /** Whether to generate code that keeps injectors backwards compatible. */
-  backwardsCompatibleConstructors?: boolean;
-
-  /** Whether to migrate abstract classes. */
-  migrateAbstractClasses?: boolean;
-
-  /** Whether to make the return type of `@Optinal()` parameters to be non-nullable. */
-  nonNullableOptional?: boolean;
-
-  /**
-   * Internal-only option that determines whether the migration should try to move the
-   * initializers of class members from the constructor back into the member itself. E.g.
-   *
-   * ```
-   * // Before
-   * private foo;
-   *
-   * constructor(@Inject(BAR) private bar: Bar) {
-   *   this.foo = this.bar.getValue();
-   * }
-   *
-   * // After
-   * private bar = inject(BAR);
-   * private foo = this.bar.getValue();
-   * ```
-   */
-  _internalCombineMemberInitializers?: boolean;
-}
-
 /**
  * Migrates all of the classes in a `SourceFile` away from constructor injection.
  * @param sourceFile File to be migrated.
@@ -72,7 +42,7 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions
   // 2. All the necessary information for this migration is local so using a file-specific type
   //    checker should speed up the lookups.
   const localTypeChecker = getLocalTypeChecker(sourceFile);
-  const analysis = analyzeFile(sourceFile, localTypeChecker);
+  const analysis = analyzeFile(sourceFile, localTypeChecker, options);
 
   if (analysis === null || analysis.classes.length === 0) {
     return [];
@@ -148,14 +118,6 @@ function migrateClass(
   printer: ts.Printer,
   tracker: ChangeTracker,
 ): void {
-  const isAbstract = !!node.modifiers?.some((m) => m.kind === ts.SyntaxKind.AbstractKeyword);
-
-  // Don't migrate abstract classes by default, because
-  // their parameters aren't guaranteed to be injectable.
-  if (isAbstract && !options.migrateAbstractClasses) {
-    return;
-  }
-
   const sourceFile = node.getSourceFile();
   const unusedParameters = getConstructorUnusedParameters(
     constructor,

packages/core/schematics/test/inject_migration_spec.ts

@@ -2062,5 +2062,25 @@ describe('inject migration', () => {
         `}`,
       ]);
     });
+
+    // There's an identical test above, but we want to ensure that the
+    // internal migration doesn't touch abstract classes either.
+    it('should not migrate abstract classes by default in the internal migration', async () => {
+      const initialContent = [
+        `import { Directive } from '@angular/core';`,
+        `import { Foo } from 'foo';`,
+        ``,
+        `@Directive()`,
+        `abstract class MyDir {`,
+        `  constructor(private foo: Foo) {}`,
+        `}`,
+      ].join('\n');
+
+      writeFile('/dir.ts', initialContent);
+
+      await runInternalMigration();
+
+      expect(tree.readContent('/dir.ts')).toBe(initialContent);
+    });
   });
 });