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
This commit is contained in:
Kristiyan Kostadinov
2023-01-27 13:51:13 +01:00
committed by Jessica Janiuk
parent c865b8b59c
commit f82bdc4b01
2 changed files with 138 additions and 10 deletions
@@ -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;
}
@@ -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';