fix(migrations): don't delete classes that may provide dependencies transitively (#48866)

crisbeto · thePunderWoman · commit f82bdc4b01f9 · 2023-01-27T16:55:20.000Z
Fixes that we would incorrectly remove a module that imports another module which has providers. This is a follow-up from the following discussion: #48832 (comment) PR Close #48866
diff --git 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<string, [start: number, end: number][]>;
@@ -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<ts.Node>, 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
@@ -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<any>('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';
