diff --git a/packages/core/schematics/ng-generate/inject-migration/analysis.ts b/packages/core/schematics/ng-generate/inject-migration/analysis.ts index 5c043a4f24f..ca197ed561b 100644 --- a/packages/core/schematics/ng-generate/inject-migration/analysis.ts +++ b/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, diff --git a/packages/core/schematics/ng-generate/inject-migration/index.ts b/packages/core/schematics/ng-generate/inject-migration/index.ts index 136ec20f3ed..26007dca8a4 100644 --- a/packages/core/schematics/ng-generate/inject-migration/index.ts +++ b/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; diff --git a/packages/core/schematics/ng-generate/inject-migration/migration.ts b/packages/core/schematics/ng-generate/inject-migration/migration.ts index 0ee6bf94ba8..8cb1cf50944 100644 --- a/packages/core/schematics/ng-generate/inject-migration/migration.ts +++ b/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, diff --git a/packages/core/schematics/test/inject_migration_spec.ts b/packages/core/schematics/test/inject_migration_spec.ts index 77845d04dd2..25f0cc19f87 100644 --- a/packages/core/schematics/test/inject_migration_spec.ts +++ b/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); + }); }); });