From 8f6affdd64c6022c6a96fddac564c0ec05c5da9b Mon Sep 17 00:00:00 2001 From: Jessica Janiuk Date: Wed, 22 Nov 2023 11:48:09 -0500 Subject: [PATCH] fix(migrations): fixes control flow migration common module removal (#53076) Common module removal would not happen when a component used a templateUrl due to the checks being in separate files. This change passes the removal analysis back to the original source file to safely remove CommonModule. PR Close #53076 --- .../control-flow-migration/index.ts | 15 ++++++-- .../control-flow-migration/migration.ts | 12 ++++++- .../control-flow-migration/types.ts | 7 +++- .../control-flow-migration/util.ts | 27 ++++++++++---- .../test/control_flow_migration_spec.ts | 35 +++++++++++++++++++ 5 files changed, 85 insertions(+), 11 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 3757bbb872368..52b892635324a 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/index.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/index.ts @@ -78,7 +78,12 @@ function runControlFlowMigration( analyze(sourceFile, analysis); } - for (const [path, file] of analysis) { + // sort files with .html files first + // this ensures class files know if it's safe to remove CommonModule + const paths = sortFilePaths([...analysis.keys()]); + + for (const path of paths) { + const file = analysis.get(path)!; const ranges = file.getSortedRanges(); const relativePath = relative(basePath, path); const content = tree.readText(relativePath); @@ -89,7 +94,7 @@ function runControlFlowMigration( const length = (end ?? content.length) - start; const {migrated, errors} = - migrateTemplate(template, type, node, file, schematicOptions.format); + migrateTemplate(template, type, node, file, schematicOptions.format, analysis); if (migrated !== null) { update.remove(start, length); @@ -113,6 +118,12 @@ function runControlFlowMigration( return errorList; } +function sortFilePaths(names: string[]): string[] { + const templateFiles = names.filter(n => n.endsWith('.html')); + const classFiles = names.filter(n => !n.endsWith('.html')); + return [...templateFiles, ...classFiles]; +} + function generateErrorMessage(path: string, errors: MigrateError[]): string { let errorMessage = `Template "${path}" encountered ${errors.length} errors during migration:\n`; errorMessage += errors.map(e => ` - ${e.type}: ${e.error}\n`); 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 d37ca31698409..8951d8fcb05a6 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/migration.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/migration.ts @@ -20,7 +20,8 @@ import {canRemoveCommonModule, formatTemplate, processNgTemplates, removeImports */ export function migrateTemplate( template: string, templateType: string, node: ts.Node, file: AnalyzedFile, - format: boolean = true): {migrated: string, errors: MigrateError[]} { + format: boolean = true, + analyzedFiles: Map|null): {migrated: string, errors: MigrateError[]} { let errors: MigrateError[] = []; let migrated = template; if (templateType === 'template' || templateType === 'templateUrl') { @@ -36,6 +37,15 @@ export function migrateTemplate( } file.removeCommonModule = canRemoveCommonModule(template); + // when migrating an external template, we have to pass back + // whether it's safe to remove the CommonModule to the + // original component class source file + if (templateType === 'templateUrl' && analyzedFiles !== null && + analyzedFiles.has(file.sourceFilePath)) { + const componentFile = analyzedFiles.get(file.sourceFilePath)!; + componentFile.removeCommonModule = file.removeCommonModule; + } + errors = [ ...ifResult.errors, ...forResult.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 72b4b3b8a180a..313d6cc7003e7 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/types.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/types.ts @@ -205,9 +205,11 @@ export class Template { export class AnalyzedFile { private ranges: Range[] = []; removeCommonModule = false; + sourceFilePath: string = ''; /** Returns the ranges in the order in which they should be migrated. */ getSortedRanges(): Range[] { + // templates first for checking on whether certain imports can be safely removed const templateRanges = this.ranges.slice() .filter(x => x.type !== 'import') .sort((aStart, bStart) => bStart.start - aStart.start); @@ -223,11 +225,14 @@ export class AnalyzedFile { * @param analyzedFiles Map keeping track of all the analyzed files. * @param range Range to be added. */ - static addRange(path: string, analyzedFiles: Map, range: Range): void { + static addRange( + path: string, sourceFilePath: string, analyzedFiles: Map, + range: Range): void { let analysis = analyzedFiles.get(path); if (!analysis) { analysis = new AnalyzedFile(); + analysis.sourceFilePath = sourceFilePath; analyzedFiles.set(path, analysis); } 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 d054281a12f2f..c3cd8c202966f 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/util.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/util.ts @@ -77,11 +77,21 @@ function updateImportClause(clause: ts.ImportClause, removeCommonModule: boolean } function updateClassImports( - propAssignment: ts.PropertyAssignment, removeCommonModule: boolean): string { - const printer = ts.createPrinter(); + propAssignment: ts.PropertyAssignment, removeCommonModule: boolean): string|null { + // removeComments is set to true to prevent duplication of comments + // when the import declaration is at the top of the file, but right after a comment + // without this, the comment gets duplicated when the declaration is updated. + // the typescript AST includes that preceding comment as part of the import declaration full text. + const printer = ts.createPrinter({ + removeComments: true, + }); const importList = propAssignment.initializer as ts.ArrayLiteralExpression; const removals = removeCommonModule ? importWithCommonRemovals : importRemovals; const elements = importList.elements.filter(el => !removals.includes(el.getText())); + if (elements.length === importList.elements.length) { + // nothing changed + return null; + } const updatedElements = ts.factory.updateArrayLiteralExpression(importList, elements); const updatedAssignment = ts.factory.updatePropertyAssignment(propAssignment, propAssignment.name, updatedElements); @@ -101,7 +111,7 @@ function analyzeImportDeclarations( clause.namedBindings.elements.filter(el => importWithCommonRemovals.includes(el.getText())); if (elements.length > 0) { AnalyzedFile.addRange( - sourceFile.fileName, analyzedFiles, + sourceFile.fileName, sourceFile.fileName, analyzedFiles, {start: node.getStart(), end: node.getEnd(), node, type: 'import'}); } } @@ -139,7 +149,7 @@ function analyzeDecorators( switch (prop.name.text) { case 'template': // +1/-1 to exclude the opening/closing characters from the range. - AnalyzedFile.addRange(sourceFile.fileName, analyzedFiles, { + AnalyzedFile.addRange(sourceFile.fileName, sourceFile.fileName, analyzedFiles, { start: prop.initializer.getStart() + 1, end: prop.initializer.getEnd() - 1, node: prop, @@ -148,7 +158,7 @@ function analyzeDecorators( break; case 'imports': - AnalyzedFile.addRange(sourceFile.fileName, analyzedFiles, { + AnalyzedFile.addRange(sourceFile.fileName, sourceFile.fileName, analyzedFiles, { start: prop.name.getStart(), end: prop.initializer.getEnd(), node: prop, @@ -160,7 +170,9 @@ function analyzeDecorators( // 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, {start: 0, node: prop, type: 'templateUrl'}); + AnalyzedFile.addRange( + path, sourceFile.fileName, analyzedFiles, + {start: 0, node: prop, type: 'templateUrl'}); } break; } @@ -353,7 +365,8 @@ export function canRemoveCommonModule(template: string): boolean { export function removeImports( template: string, node: ts.Node, removeCommonModule: boolean): string { if (template.startsWith('imports') && ts.isPropertyAssignment(node)) { - return updateClassImports(node, removeCommonModule); + const updatedImport = updateClassImports(node, removeCommonModule); + return updatedImport ?? template; } else if (ts.isImportDeclaration(node) && checkIfShouldChange(node, removeCommonModule)) { return updateImportDeclaration(node, removeCommonModule); } diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts index fcad5ac3c2f05..aef63d6e21ea6 100644 --- a/packages/core/schematics/test/control_flow_migration_spec.ts +++ b/packages/core/schematics/test/control_flow_migration_spec.ts @@ -4020,6 +4020,41 @@ describe('control flow migration', () => { expect(actual).toBe(expected); }); + + it('should remove common module post migration if using external template', async () => { + writeFile('/comp.ts', [ + `import {Component} from '@angular/core';`, + `import {CommonModule} from '@angular/common';\n`, + `@Component({`, + ` imports: [CommonModule],`, + ` templateUrl: './comp.html',`, + `})`, + `class Comp {`, + ` toggle = false;`, + `}`, + ].join('\n')); + + writeFile('/comp.html', [ + `
`, + `Content here`, + `
`, + ].join('\n')); + + await runMigration(); + const actual = tree.readContent('/comp.ts'); + const expected = [ + `import {Component} from '@angular/core';\n\n`, + `@Component({`, + ` imports: [],`, + ` templateUrl: './comp.html',`, + `})`, + `class Comp {`, + ` toggle = false;`, + `}`, + ].join('\n'); + + expect(actual).toBe(expected); + }); }); describe('no migration needed', () => {