fix(migrations): fixes control flow migration common module removal (#53076)

thePunderWoman · pkozlowski-opensource · commit 8f6affdd64c6 · 2023-11-28T14:03:29.000+01:00
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
diff --git 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
@@ -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<string, AnalyzedFile>|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
@@ -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<string, AnalyzedFile>, range: Range): void {
+  static addRange(
+      path: string, sourceFilePath: string, analyzedFiles: Map<string, AnalyzedFile>,
+      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
@@ -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
@@ -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', [
+        `<div>`,
+        `<span *ngIf="show">Content here</span>`,
+        `</div>`,
+      ].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', () => {
