From aa2d815648dbf3303cfe72bf976a4a87de406ee0 Mon Sep 17 00:00:00 2001 From: Jessica Janiuk Date: Thu, 9 Nov 2023 17:58:10 -0500 Subject: [PATCH] fix(migrations): Add support for removing imports post migration (#52763) This update removes imports from component decorators and at the top of the files. It only removes standalone imports though. It does not remove CommonModule if that is the only import. PR Close #52763 --- .../control-flow-migration/index.ts | 4 +- .../control-flow-migration/migration.ts | 35 ++-- .../control-flow-migration/types.ts | 89 +++++++- .../control-flow-migration/util.ts | 190 ++++++++++++++---- .../test/control_flow_migration_spec.ts | 128 +++++++++++- 5 files changed, 386 insertions(+), 60 deletions(-) diff --git a/packages/core/schematics/ng-generate/control-flow-migration/index.ts b/packages/core/schematics/ng-generate/control-flow-migration/index.ts index f5664072185ba..6b3fcef557ed0 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/index.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/index.ts @@ -82,11 +82,11 @@ function runControlFlowMigration( const content = tree.readText(relativePath); const update = tree.beginUpdate(relativePath); - for (const [start, end] of ranges) { + for (const {start, end, node, type} of ranges) { const template = content.slice(start, end); const length = (end ?? content.length) - start; - const {migrated, errors} = migrateTemplate(template); + const {migrated, errors} = migrateTemplate(template, type, node, file); if (migrated !== null) { update.remove(start, length); diff --git a/packages/core/schematics/ng-generate/control-flow-migration/migration.ts b/packages/core/schematics/ng-generate/control-flow-migration/migration.ts index 3d8568015bde7..6f68b16d0a3de 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/migration.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/migration.ts @@ -6,26 +6,37 @@ * found in the LICENSE file at https://angular.io/license */ +import ts from 'typescript'; + import {migrateFor} from './fors'; import {migrateIf} from './ifs'; import {migrateSwitch} from './switches'; -import {MigrateError} from './types'; -import {processNgTemplates} from './util'; +import {AnalyzedFile, MigrateError} from './types'; +import {canRemoveCommonModule, processNgTemplates, removeImports} from './util'; /** * Actually migrates a given template to the new syntax */ -export function migrateTemplate(template: string): {migrated: string, errors: MigrateError[]} { - const ifResult = migrateIf(template); - const forResult = migrateFor(ifResult.migrated); - const switchResult = migrateSwitch(forResult.migrated); +export function migrateTemplate( + template: string, templateType: string, node: ts.Node, + file: AnalyzedFile): {migrated: string, errors: MigrateError[]} { + let errors: MigrateError[] = []; + let migrated = template; + if (templateType === 'template') { + const ifResult = migrateIf(template); + const forResult = migrateFor(ifResult.migrated); + const switchResult = migrateSwitch(forResult.migrated); + migrated = processNgTemplates(switchResult.migrated); + file.removeCommonModule = canRemoveCommonModule(template); - const migrated = processNgTemplates(switchResult.migrated); + errors = [ + ...ifResult.errors, + ...forResult.errors, + ...switchResult.errors, + ]; + } else { + migrated = removeImports(template, node, file.removeCommonModule); + } - const errors = [ - ...ifResult.errors, - ...forResult.errors, - ...switchResult.errors, - ]; return {migrated, errors}; } diff --git a/packages/core/schematics/ng-generate/control-flow-migration/types.ts b/packages/core/schematics/ng-generate/control-flow-migration/types.ts index 2c9c01cad7f73..37fa9694b48cf 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/types.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/types.ts @@ -6,15 +6,59 @@ * found in the LICENSE file at https://angular.io/license */ -import {Attribute, Element, RecursiveVisitor} from '@angular/compiler'; +import {Attribute, Element, RecursiveVisitor, Text} from '@angular/compiler'; +import ts from 'typescript'; export const ngtemplate = 'ng-template'; +function allFormsOf(selector: string): string[] { + return [ + selector, + `*${selector}`, + `[${selector}]`, + ]; +} + +const commonModuleDirectives = new Set([ + ...allFormsOf('ngComponentOutlet'), + ...allFormsOf('ngTemplateOutlet'), + ...allFormsOf('ngClass'), + ...allFormsOf('ngPlural'), + ...allFormsOf('ngPluralCase'), + ...allFormsOf('ngStyle'), + ...allFormsOf('ngTemplateOutlet'), + ...allFormsOf('ngComponentOutlet'), +]); + +function pipeMatchRegExpFor(name: string): RegExp { + return new RegExp(`\\|\\s*${name}`); +} + +const commonModulePipes = [ + 'date', + 'async', + 'currency', + 'number', + 'i18nPlural', + 'i18nSelect', + 'json', + 'keyvalue', + 'slice', + 'lowercase', + 'uppercase', + 'titlecase', + 'percent', + 'titlecase', +].map(name => pipeMatchRegExpFor(name)); + /** * Represents a range of text within a file. Omitting the end * means that it's until the end of the file. */ -type Range = [start: number, end?: number]; +type Range = { + start: number, + end?: number, node: ts.Node, type: string, +}; export type Offsets = { pre: number, @@ -104,10 +148,17 @@ export class Template { /** Represents a file that was analyzed by the migration. */ export class AnalyzedFile { private ranges: Range[] = []; + removeCommonModule = false; /** Returns the ranges in the order in which they should be migrated. */ getSortedRanges(): Range[] { - return this.ranges.slice().sort(([aStart], [bStart]) => bStart - aStart); + const templateRanges = this.ranges.slice() + .filter(x => x.type === 'template') + .sort((aStart, bStart) => bStart.start - aStart.start); + const importRanges = this.ranges.slice() + .filter(x => x.type === 'import') + .sort((aStart, bStart) => bStart.start - aStart.start); + return [...templateRanges, ...importRanges]; } /** @@ -125,7 +176,7 @@ export class AnalyzedFile { } const duplicate = - analysis.ranges.find(current => current[0] === range[0] && current[1] === range[1]); + analysis.ranges.find(current => current.start === range.start && current.end === range.end); if (!duplicate) { analysis.ranges.push(range); @@ -133,6 +184,36 @@ export class AnalyzedFile { } } +/** Finds all non-control flow elements from common module. */ +export class CommonCollector extends RecursiveVisitor { + count = 0; + + override visitElement(el: Element): void { + if (el.attrs.length > 0) { + for (const attr of el.attrs) { + if (this.hasDirectives(attr.name) || this.hasPipes(attr.value)) { + this.count++; + } + } + } + super.visitElement(el, null); + } + + override visitText(ast: Text) { + if (this.hasPipes(ast.value)) { + this.count++; + } + } + + private hasDirectives(input: string): boolean { + return commonModuleDirectives.has(input); + } + + private hasPipes(input: string): boolean { + return commonModulePipes.some(regexp => regexp.test(input)); + } +} + /** Finds all elements with ngif structural directives. */ export class ElementCollector extends RecursiveVisitor { readonly elements: ElementToMigrate[] = []; diff --git a/packages/core/schematics/ng-generate/control-flow-migration/util.ts b/packages/core/schematics/ng-generate/control-flow-migration/util.ts index a8c1d2b42d4a9..d022b73f7a610 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/util.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/util.ts @@ -10,7 +10,10 @@ import {Attribute, HtmlParser, ParseTreeResult, visitAll} from '@angular/compile import {dirname, join} from 'path'; import ts from 'typescript'; -import {AnalyzedFile, ElementCollector, ElementToMigrate, Template, TemplateCollector} from './types'; +import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, Template, TemplateCollector} from './types'; + +const importRemovals = ['NgIf', 'NgFor', 'NgSwitch', 'NgSwitchCase', 'NgSwitchDefault']; +const importWithCommonRemovals = [...importRemovals, 'CommonModule']; /** * Analyzes a source file to find file that need to be migrated and the text ranges within them. @@ -19,48 +22,129 @@ import {AnalyzedFile, ElementCollector, ElementToMigrate, Template, TemplateColl */ export function analyze(sourceFile: ts.SourceFile, analyzedFiles: Map) { forEachClass(sourceFile, node => { - // Note: we have a utility to resolve the Angular decorators from a class declaration already. - // We don't use it here, because it requires access to the type checker which makes it more - // time-consuming to run internally. - const decorator = ts.getDecorators(node)?.find(dec => { - return ts.isCallExpression(dec.expression) && ts.isIdentifier(dec.expression.expression) && - dec.expression.expression.text === 'Component'; - }) as (ts.Decorator & {expression: ts.CallExpression}) | - undefined; - - const metadata = decorator && decorator.expression.arguments.length > 0 && - ts.isObjectLiteralExpression(decorator.expression.arguments[0]) ? - decorator.expression.arguments[0] : - null; - - if (!metadata) { - return; + if (ts.isClassDeclaration(node)) { + analyzeDecorators(node, sourceFile, analyzedFiles); + } else { + analyzeImportDeclarations(node, sourceFile, analyzedFiles); } + }); +} - for (const prop of metadata.properties) { - // All the properties we care about should have static - // names and be initialized to a static string. - if (!ts.isPropertyAssignment(prop) || !ts.isStringLiteralLike(prop.initializer) || - (!ts.isIdentifier(prop.name) && !ts.isStringLiteralLike(prop.name))) { - continue; - } +function updateImportDeclaration(decl: ts.ImportDeclaration, removeCommonModule: boolean): string { + const clause = decl.getChildAt(1) as ts.ImportClause; + const updatedClause = updateImportClause(clause, removeCommonModule); + if (updatedClause === null) { + return ''; + } + const printer = ts.createPrinter(); + const updated = ts.factory.updateImportDeclaration( + decl, decl.modifiers, updatedClause, decl.moduleSpecifier, undefined); + return printer.printNode(ts.EmitHint.Unspecified, updated, clause.getSourceFile()); +} - switch (prop.name.text) { - case 'template': - // +1/-1 to exclude the opening/closing characters from the range. - AnalyzedFile.addRange( - sourceFile.fileName, analyzedFiles, - [prop.initializer.getStart() + 1, prop.initializer.getEnd() - 1]); - break; +function updateImportClause(clause: ts.ImportClause, removeCommonModule: boolean): ts.ImportClause| + null { + if (clause.namedBindings && ts.isNamedImports(clause.namedBindings)) { + const removals = removeCommonModule ? importWithCommonRemovals : importRemovals; + const elements = clause.namedBindings.elements.filter(el => !removals.includes(el.getText())); + if (elements.length === 0) { + return null; + } + clause = ts.factory.updateImportClause( + clause, clause.isTypeOnly, clause.name, ts.factory.createNamedImports(elements)); + } + return clause; +} - case 'templateUrl': - // Leave the end as undefined which means that the range is until the end of the file. +function updateClassImports( + propAssignment: ts.PropertyAssignment, removeCommonModule: boolean): string { + const printer = ts.createPrinter(); + const importList = propAssignment.initializer as ts.ArrayLiteralExpression; + const removals = removeCommonModule ? importWithCommonRemovals : importRemovals; + const elements = importList.elements.filter(el => !removals.includes(el.getText())); + const updatedElements = ts.factory.updateArrayLiteralExpression(importList, elements); + const updatedAssignment = + ts.factory.updatePropertyAssignment(propAssignment, propAssignment.name, updatedElements); + return printer.printNode( + ts.EmitHint.Unspecified, updatedAssignment, updatedAssignment.getSourceFile()); +} + +function analyzeImportDeclarations( + node: ts.ImportDeclaration, sourceFile: ts.SourceFile, + analyzedFiles: Map) { + if (node.getText().indexOf('@angular/common') === -1) { + return; + } + const clause = node.getChildAt(1) as ts.ImportClause; + if (clause.namedBindings && ts.isNamedImports(clause.namedBindings)) { + const elements = + clause.namedBindings.elements.filter(el => importWithCommonRemovals.includes(el.getText())); + if (elements.length > 0) { + AnalyzedFile.addRange( + sourceFile.fileName, analyzedFiles, + {start: node.getStart(), end: node.getEnd(), node, type: 'import'}); + } + } +} + +function analyzeDecorators( + node: ts.ClassDeclaration, sourceFile: ts.SourceFile, + analyzedFiles: Map) { + // Note: we have a utility to resolve the Angular decorators from a class declaration already. + // We don't use it here, because it requires access to the type checker which makes it more + // time-consuming to run internally. + const decorator = ts.getDecorators(node)?.find(dec => { + return ts.isCallExpression(dec.expression) && ts.isIdentifier(dec.expression.expression) && + dec.expression.expression.text === 'Component'; + }) as (ts.Decorator & {expression: ts.CallExpression}) | + undefined; + + const metadata = decorator && decorator.expression.arguments.length > 0 && + ts.isObjectLiteralExpression(decorator.expression.arguments[0]) ? + decorator.expression.arguments[0] : + null; + + if (!metadata) { + return; + } + + for (const prop of metadata.properties) { + // All the properties we care about should have static + // names and be initialized to a static string. + if (!ts.isPropertyAssignment(prop) || + (!ts.isIdentifier(prop.name) && !ts.isStringLiteralLike(prop.name))) { + continue; + } + + switch (prop.name.text) { + case 'template': + // +1/-1 to exclude the opening/closing characters from the range. + AnalyzedFile.addRange(sourceFile.fileName, analyzedFiles, { + start: prop.initializer.getStart() + 1, + end: prop.initializer.getEnd() - 1, + node: prop, + type: 'template' + }); + break; + + case 'imports': + AnalyzedFile.addRange(sourceFile.fileName, analyzedFiles, { + start: prop.name.getStart(), + end: prop.initializer.getEnd(), + node: prop, + type: 'import' + }); + break; + + case 'templateUrl': + // Leave the end as undefined which means that the range is until the end of the file. + if (ts.isStringLiteralLike(prop.initializer)) { const path = join(dirname(sourceFile.fileName), prop.initializer.text); - AnalyzedFile.addRange(path, analyzedFiles, [0]); - break; - } + AnalyzedFile.addRange(path, analyzedFiles, {start: 0, node: prop, type: 'template'}); + } + break; } - }); + } } /** @@ -216,6 +300,33 @@ export function processNgTemplates(template: string): string { return template; } +/** + * determines if the CommonModule can be safely removed from imports + */ +export function canRemoveCommonModule(template: string): boolean { + const parsed = parseTemplate(template); + let removeCommonModule = false; + if (parsed !== null) { + const visitor = new CommonCollector(); + visitAll(visitor, parsed.rootNodes); + removeCommonModule = visitor.count === 0; + } + return removeCommonModule; +} + +/** + * removes imports from template imports and import declarations + */ +export function removeImports( + template: string, node: ts.Node, removeCommonModule: boolean): string { + if (template.startsWith('imports') && ts.isPropertyAssignment(node)) { + return updateClassImports(node, removeCommonModule); + } else if (ts.isImportDeclaration(node)) { + return updateImportDeclaration(node, removeCommonModule); + } + return template; +} + /** * retrieves the original block of text in the template for length comparison during migration * processing @@ -283,9 +394,10 @@ export function getMainBlock(etm: ElementToMigrate, tmpl: string, offset: number } /** Executes a callback on each class declaration in a file. */ -function forEachClass(sourceFile: ts.SourceFile, callback: (node: ts.ClassDeclaration) => void) { +function forEachClass( + sourceFile: ts.SourceFile, callback: (node: ts.ClassDeclaration|ts.ImportDeclaration) => void) { sourceFile.forEachChild(function walk(node) { - if (ts.isClassDeclaration(node)) { + if (ts.isClassDeclaration(node) || ts.isImportDeclaration(node)) { callback(node); } node.forEachChild(walk); diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts index b7f876ffe6c6a..124de02f69f23 100644 --- a/packages/core/schematics/test/control_flow_migration_spec.ts +++ b/packages/core/schematics/test/control_flow_migration_spec.ts @@ -105,14 +105,12 @@ describe('control flow migration', () => { }); it('should only migrate the paths that were passed in', async () => { - let error: string|null = null; - writeFile('comp.ts', ` import {Component} from '@angular/core'; import {NgIf} from '@angular/common'; @Component({ - imports: [NgIf], + imports: [NgIf, NgFor,NgSwitch,NgSwitchCase ,NgSwitchDefault], template: \`
This should be hidden
\` }) class Comp { @@ -139,7 +137,11 @@ describe('control flow migration', () => { expect(migratedContent) .toContain('template: `
@if (toggle) {This should be hidden}
`'); + expect(migratedContent).toContain('imports: []'); + expect(migratedContent).not.toContain(`import {NgIf} from '@angular/common';`); expect(skippedContent).toContain('template: `
Show me
`'); + expect(skippedContent).toContain('imports: [NgIf]'); + expect(skippedContent).toContain(`import {NgIf} from '@angular/common';`); }); }); @@ -2730,6 +2732,126 @@ describe('control flow migration', () => { }); }); + describe('imports', () => { + it('should remove common module imports post migration', async () => { + writeFile('/comp.ts', [ + `import {Component} from '@angular/core';`, + `import {NgIf} from '@angular/common';`, + `@Component({`, + ` imports: [NgIf],`, + ` template: \`
shrug
\``, + `})`, + `class Comp {`, + ` toggle = false;`, + `}`, + ].join('\n')); + + await runMigration(); + const actual = tree.readContent('/comp.ts'); + const expected = [ + `import {Component} from '@angular/core';\n`, + `@Component({`, + ` imports: [],`, + ` template: \`
@if (toggle) {shrug}
\``, + `})`, + `class Comp {`, + ` toggle = false;`, + `}`, + ].join('\n'); + + expect(actual).toBe(expected); + }); + + it('should leave non-cf common module imports post migration', async () => { + writeFile('/comp.ts', [ + `import {Component} from '@angular/core';`, + `import {NgIf, AsyncPipe} from '@angular/common';\n`, + `@Component({`, + ` imports: [NgIf, AsyncPipe],`, + ` template: \`
shrug
\``, + `})`, + `class Comp {`, + ` toggle = false;`, + `}`, + ].join('\n')); + + await runMigration(); + const actual = tree.readContent('/comp.ts'); + const expected = [ + `import {Component} from '@angular/core';`, + `import { AsyncPipe } from '@angular/common';\n`, + `@Component({`, + ` imports: [AsyncPipe],`, + ` template: \`
@if (toggle) {shrug}
\``, + `})`, + `class Comp {`, + ` toggle = false;`, + `}`, + ].join('\n'); + + expect(actual).toBe(expected); + }); + + it('should remove common module post migration', async () => { + writeFile('/comp.ts', [ + `import {Component} from '@angular/core';`, + `import {CommonModule} from '@angular/common';`, + `@Component({`, + ` imports: [CommonModule],`, + ` template: \`
shrug
\``, + `})`, + `class Comp {`, + ` toggle = false;`, + `}`, + ].join('\n')); + + await runMigration(); + const actual = tree.readContent('/comp.ts'); + const expected = [ + `import {Component} from '@angular/core';\n`, + `@Component({`, + ` imports: [],`, + ` template: \`
@if (toggle) {shrug}
\``, + `})`, + `class Comp {`, + ` toggle = false;`, + `}`, + ].join('\n'); + + expect(actual).toBe(expected); + }); + + it('should leave common module post migration if other common module deps exist', async () => { + writeFile('/comp.ts', [ + `import {Component} from '@angular/core';`, + `import {CommonModule} from '@angular/common';\n`, + `@Component({`, + ` imports: [CommonModule],`, + ` template: \`
{{ shrug | lowercase }}
\``, + `})`, + `class Comp {`, + ` toggle = false;`, + `}`, + ].join('\n')); + + await runMigration(); + const actual = tree.readContent('/comp.ts'); + const expected = [ + `import {Component} from '@angular/core';`, + `import { CommonModule } from '@angular/common';\n`, + `@Component({`, + ` imports: [CommonModule],`, + ` template: \`
@if (toggle) {{{ shrug | lowercase }}}
\``, + `})`, + `class Comp {`, + ` toggle = false;`, + `}`, + ].join('\n'); + + expect(actual).toBe(expected); + }); + }); + describe('no migration needed', () => { it('should do nothing when no control flow is present', async () => { writeFile('/comp.ts', `