From f82bdc4b01f93a7103870449d37da61cc4c4f179 Mon Sep 17 00:00:00 2001 From: Kristiyan Kostadinov Date: Fri, 27 Jan 2023 13:51:13 +0100 Subject: [PATCH] fix(migrations): don't delete classes that may provide dependencies transitively (#48866) Fixes that we would incorrectly remove a module that imports another module which has providers. This is a follow-up from the following discussion: https://github.com/angular/angular/pull/48832#discussion_r1086623514 PR Close #48866 --- .../standalone-migration/prune-modules.ts | 40 +++++-- .../test/standalone_migration_spec.ts | 108 ++++++++++++++++++ 2 files changed, 138 insertions(+), 10 deletions(-) diff --git a/packages/core/schematics/ng-generate/standalone-migration/prune-modules.ts b/packages/core/schematics/ng-generate/standalone-migration/prune-modules.ts index afbe97be2e1..4fed86b1947 100644 --- a/packages/core/schematics/ng-generate/standalone-migration/prune-modules.ts +++ b/packages/core/schematics/ng-generate/standalone-migration/prune-modules.ts @@ -9,10 +9,10 @@ import {NgtscProgram} from '@angular/compiler-cli'; import ts from 'typescript'; -import {getAngularDecorators} from '../../utils/ng_decorators'; +import {getAngularDecorators, NgDecorator} from '../../utils/ng_decorators'; import {closestNode} from '../../utils/typescript/nodes'; -import {ChangeTracker, createLanguageService, findLiteralProperty, getNodeLookup, offsetsToNodes, UniqueItemTracker} from './util'; +import {ChangeTracker, createLanguageService, findClassDeclaration, findLiteralProperty, getNodeLookup, offsetsToNodes, UniqueItemTracker} from './util'; /** Mapping between a file name and spans for node references inside of it. */ type ReferencesByFile = Map; @@ -199,9 +199,8 @@ function removeExportReferences( * @param typeChecker */ function canRemoveClass(node: ts.ClassDeclaration, typeChecker: ts.TypeChecker): boolean { - const decorator = getAngularDecorators(typeChecker, ts.getDecorators(node) || []) - .find(decorator => decorator.name === 'NgModule') - ?.node; + const decorator = findNgModuleDecorator(node, typeChecker)?.node; + // We can't remove a declaration if it's not a valid `NgModule`. if (!decorator || !ts.isCallExpression(decorator.expression)) { return false; @@ -227,11 +226,25 @@ function canRemoveClass(node: ts.ClassDeclaration, typeChecker: ts.TypeChecker): const literal = decorator.expression.arguments[0] as ts.ObjectLiteralExpression; const imports = findLiteralProperty(literal, 'imports'); - // We can't remove classes where the `imports` contain something different - // from an identifier, because it may be a `ModuleWithProviders`. - if (imports && isNonEmptyNgModuleProperty(imports) && - imports.initializer.elements.some(el => !ts.isIdentifier(el))) { - return false; + if (imports && isNonEmptyNgModuleProperty(imports)) { + // We can't remove the class if at least one import isn't identifier, because it may be a + // `ModuleWithProviders` which is the equivalent of having something in the `providers` array. + for (const dep of imports.initializer.elements) { + if (!ts.isIdentifier(dep)) { + return false; + } + + const depDeclaration = findClassDeclaration(dep, typeChecker); + const depNgModule = + depDeclaration ? findNgModuleDecorator(depDeclaration, typeChecker) : null; + + // If any of the dependencies of the class is an `NgModule` that can't be removed, the class + // itself can't be removed either, because it may be part of a transitive dependency chain. + if (depDeclaration !== null && depNgModule !== null && + !canRemoveClass(depDeclaration, typeChecker)) { + return false; + } + } } // We can't remove classes that have any `declarations`, `providers` or `bootstrap` elements. @@ -364,3 +377,10 @@ function addRemovalTodos(nodes: Set, tracker: ChangeTracker) { ` /* TODO(standalone-migration): clean up removed NgModule reference manually. */ `); } } + +/** Finds the `NgModule` decorator in a class, if it exists. */ +function findNgModuleDecorator(node: ts.ClassDeclaration, typeChecker: ts.TypeChecker): NgDecorator| + null { + const decorators = getAngularDecorators(typeChecker, ts.getDecorators(node) || []); + return decorators.find(decorator => decorator.name === 'NgModule') || null; +} diff --git a/packages/core/schematics/test/standalone_migration_spec.ts b/packages/core/schematics/test/standalone_migration_spec.ts index c5adb341067..326a9d7fdab 100644 --- a/packages/core/schematics/test/standalone_migration_spec.ts +++ b/packages/core/schematics/test/standalone_migration_spec.ts @@ -1413,6 +1413,114 @@ describe('standalone migration', () => { `)); }); + it('should not remove a chain of modules if a module in the chain cannot be removed because it has providers', + async () => { + const moduleAContent = ` + import {NgModule, InjectionToken} from '@angular/core'; + + export const token = new InjectionToken('token'); + + @NgModule({providers: [{provide: token, useValue: 123}]}) + export class ModuleA {} + `; + + const moduleBContent = ` + import {NgModule} from '@angular/core'; + import {ModuleA} from './a.module'; + + @NgModule({imports: [ModuleA]}) + export class ModuleB {} + `; + + const moduleCContent = ` + import {NgModule} from '@angular/core'; + import {ModuleB} from './b.module'; + + @NgModule({imports: [ModuleB]}) + export class ModuleC {} + `; + + const appModuleContent = ` + import {NgModule} from '@angular/core'; + import {MyDir} from './dir'; + import {ModuleC} from './c.module'; + + @NgModule({imports: [ModuleC], declarations: [MyDir]}) + export class AppModule {} + `; + + writeFile('a.module.ts', moduleAContent); + writeFile('b.module.ts', moduleBContent); + writeFile('c.module.ts', moduleCContent); + writeFile('app.module.ts', appModuleContent); + writeFile('dir.ts', ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[myDir]'}) + export class MyDir {} + `); + + await runMigration('prune-ng-modules'); + + expect(tree.readContent('a.module.ts')).toBe(moduleAContent); + expect(tree.readContent('b.module.ts')).toBe(moduleBContent); + expect(tree.readContent('c.module.ts')).toBe(moduleCContent); + expect(tree.readContent('app.module.ts')).toBe(appModuleContent); + }); + + it('should not remove a chain of modules if a module in the chain cannot be removed because it is importing a ModuleWithProviders', + async () => { + const moduleAContent = ` + import {NgModule} from '@angular/core'; + + @NgModule({imports: [RouterModule.forRoot([{path: '/foo'}])]}) + export class ModuleA {} + `; + + const moduleBContent = ` + import {NgModule} from '@angular/core'; + import {ModuleA} from './a.module'; + + @NgModule({imports: [ModuleA]}) + export class ModuleB {} + `; + + const moduleCContent = ` + import {NgModule} from '@angular/core'; + import {ModuleB} from './b.module'; + + @NgModule({imports: [ModuleB]}) + export class ModuleC {} + `; + + const appModuleContent = ` + import {NgModule} from '@angular/core'; + import {MyDir} from './dir'; + import {ModuleC} from './c.module'; + + @NgModule({imports: [ModuleC], declarations: [MyDir]}) + export class AppModule {} + `; + + writeFile('a.module.ts', moduleAContent); + writeFile('b.module.ts', moduleBContent); + writeFile('c.module.ts', moduleCContent); + writeFile('app.module.ts', appModuleContent); + writeFile('dir.ts', ` + import {Directive} from '@angular/core'; + + @Directive({selector: '[myDir]'}) + export class MyDir {} + `); + + await runMigration('prune-ng-modules'); + + expect(tree.readContent('a.module.ts')).toBe(moduleAContent); + expect(tree.readContent('b.module.ts')).toBe(moduleBContent); + expect(tree.readContent('c.module.ts')).toBe(moduleCContent); + expect(tree.readContent('app.module.ts')).toBe(appModuleContent); + }); + it('should not remove the module file if it contains other exported code', async () => { writeFile('app.module.ts', ` import {NgModule} from '@angular/core';