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', `