From d7afb0a086f91a8fa7e4c1258adebbfbfaa8349d Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Wed, 30 Oct 2024 09:21:20 +0100 Subject: [PATCH] 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 --- .../ng-generate/inject-migration/analysis.ts | 42 ++++++++++++++++++- .../ng-generate/inject-migration/index.ts | 3 +- .../ng-generate/inject-migration/migration.ts | 42 +------------------ .../schematics/test/inject_migration_spec.ts | 20 +++++++++ 4 files changed, 64 insertions(+), 43 deletions(-) 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); + }); }); });