fix(migrations): tweaks to formatting in control flow migration (#53058)

thePunderWoman · AndrewKushnir · commit 0fad36eff2b2 · 2023-11-20T10:46:43.000-08:00
This addresses a few minor formatting issues with the control flow migration. fixes: #53017 PR Close #53058
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/cases.ts b/packages/core/schematics/ng-generate/control-flow-migration/cases.ts
@@ -29,11 +29,12 @@ const cases = [
  * Replaces structural directive ngSwitch instances with new switch.
  * Returns null if the migration failed (e.g. there was a syntax error).
  */
-export function migrateCase(template: string): {migrated: string, errors: MigrateError[]} {
+export function migrateCase(template: string):
+    {migrated: string, errors: MigrateError[], changed: boolean} {
   let errors: MigrateError[] = [];
   let parsed = parseTemplate(template);
   if (parsed === null) {
-    return {migrated: template, errors};
+    return {migrated: template, errors, changed: false};
   }
 
   let result = template;
@@ -73,7 +74,9 @@ export function migrateCase(template: string): {migrated: string, errors: Migrat
     nestLevel = el.nestCount;
   }
 
-  return {migrated: result, errors};
+  const changed = visitor.elements.length > 0;
+
+  return {migrated: result, errors, changed};
 }
 
 function migrateNgSwitchCase(etm: ElementToMigrate, tmpl: string, offset: number): Result {
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/fors.ts b/packages/core/schematics/ng-generate/control-flow-migration/fors.ts
@@ -32,11 +32,12 @@ export const stringPairs = new Map([
  * Replaces structural directive ngFor instances with new for.
  * Returns null if the migration failed (e.g. there was a syntax error).
  */
-export function migrateFor(template: string): {migrated: string, errors: MigrateError[]} {
+export function migrateFor(template: string):
+    {migrated: string, errors: MigrateError[], changed: boolean} {
   let errors: MigrateError[] = [];
   let parsed = parseTemplate(template);
   if (parsed === null) {
-    return {migrated: template, errors};
+    return {migrated: template, errors, changed: false};
   }
 
   let result = template;
@@ -68,7 +69,9 @@ export function migrateFor(template: string): {migrated: string, errors: Migrate
     nestLevel = el.nestCount;
   }
 
-  return {migrated: result, errors};
+  const changed = visitor.elements.length > 0;
+
+  return {migrated: result, errors, changed};
 }
 
 function migrateNgFor(etm: ElementToMigrate, tmpl: string, offset: number): Result {
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/ifs.ts b/packages/core/schematics/ng-generate/control-flow-migration/ifs.ts
@@ -25,11 +25,12 @@ const ifs = [
  * Replaces structural directive ngif instances with new if.
  * Returns null if the migration failed (e.g. there was a syntax error).
  */
-export function migrateIf(template: string): {migrated: string, errors: MigrateError[]} {
+export function migrateIf(template: string):
+    {migrated: string, errors: MigrateError[], changed: boolean} {
   let errors: MigrateError[] = [];
   let parsed = parseTemplate(template);
   if (parsed === null) {
-    return {migrated: template, errors};
+    return {migrated: template, errors, changed: false};
   }
 
   let result = template;
@@ -61,7 +62,9 @@ export function migrateIf(template: string): {migrated: string, errors: MigrateE
     nestLevel = el.nestCount;
   }
 
-  return {migrated: result, errors};
+  const changed = visitor.elements.length > 0;
+
+  return {migrated: result, errors, changed};
 }
 
 function migrateNgIf(etm: ElementToMigrate, tmpl: string, offset: number): Result {
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/migration.ts b/packages/core/schematics/ng-generate/control-flow-migration/migration.ts
@@ -29,7 +29,9 @@ export function migrateTemplate(
     const switchResult = migrateSwitch(forResult.migrated);
     const caseResult = migrateCase(switchResult.migrated);
     migrated = processNgTemplates(caseResult.migrated);
-    if (format) {
+    const changed =
+        ifResult.changed || forResult.changed || switchResult.changed || caseResult.changed;
+    if (format && changed) {
       migrated = formatTemplate(migrated);
     }
     file.removeCommonModule = canRemoveCommonModule(template);
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/switches.ts b/packages/core/schematics/ng-generate/control-flow-migration/switches.ts
@@ -21,11 +21,12 @@ const switches = [
  * Replaces structural directive ngSwitch instances with new switch.
  * Returns null if the migration failed (e.g. there was a syntax error).
  */
-export function migrateSwitch(template: string): {migrated: string, errors: MigrateError[]} {
+export function migrateSwitch(template: string):
+    {migrated: string, errors: MigrateError[], changed: boolean} {
   let errors: MigrateError[] = [];
   let parsed = parseTemplate(template);
   if (parsed === null) {
-    return {migrated: template, errors};
+    return {migrated: template, errors, changed: false};
   }
 
   let result = template;
@@ -59,7 +60,9 @@ export function migrateSwitch(template: string): {migrated: string, errors: Migr
     nestLevel = el.nestCount;
   }
 
-  return {migrated: result, errors};
+  const changed = visitor.elements.length > 0;
+
+  return {migrated: result, errors, changed};
 }
 
 function migrateNgSwitch(etm: ElementToMigrate, tmpl: string, offset: number): Result {
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/util.ts b/packages/core/schematics/ng-generate/control-flow-migration/util.ts
@@ -33,13 +33,30 @@ export function analyze(sourceFile: ts.SourceFile, analyzedFiles: Map<string, An
   });
 }
 
+function checkIfShouldChange(decl: ts.ImportDeclaration, removeCommonModule: boolean) {
+  // should change if you can remove the common module
+  // if it's not safe to remove the common module
+  // and that's the only thing there, we should do nothing.
+  const clause = decl.getChildAt(1) as ts.ImportClause;
+  return !(
+      !removeCommonModule && clause.namedBindings && ts.isNamedImports(clause.namedBindings) &&
+      clause.namedBindings.elements.length === 1 &&
+      clause.namedBindings.elements[0].getText() === 'CommonModule');
+}
+
 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();
+  // 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 updated = ts.factory.updateImportDeclaration(
       decl, decl.modifiers, updatedClause, decl.moduleSpecifier, undefined);
   return printer.printNode(ts.EmitHint.Unspecified, updated, clause.getSourceFile());
@@ -326,7 +343,7 @@ 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)) {
+  } else if (ts.isImportDeclaration(node) && checkIfShouldChange(node, removeCommonModule)) {
     return updateImportDeclaration(node, removeCommonModule);
   }
   return template;
@@ -423,29 +440,29 @@ export function formatTemplate(tmpl: string): string {
 
     // regex for matching an html element opening
     // <div thing="stuff" [binding]="true"> || <div thing="stuff" [binding]="true"
-    const openElRegex = /^\s*<([a-z]+)(?![^>]*\/>)[^>]*>?/;
+    const openElRegex = /^\s*<([a-z0-9]+)(?![^>]*\/>)[^>]*>?/;
 
     // match closing block or else block
     // } | } @else
     const closeBlockRegex = /^\s*\}\s*$|^\s*\}\s\@else/;
 
     // matches closing of an html element
     // </element>
-    const closeElRegex = /\s*<\/([a-z\-]+)*>/;
+    const closeElRegex = /\s*<\/([a-z0-9\-]+)*>/;
 
     // matches closing of a self closing html element when the element is on multiple lines
     // [binding]="value" />
     const closeMultiLineElRegex = /^\s*([a-z0-9\-\[\]]+)?=?"?([^”<]+)?"?\s?\/>$/;
 
     // matches an open and close of an html element on a single line with no breaks
     // <div>blah</div>
-    const singleLineElRegex = /^\s*<([a-z]+)(?![^>]*\/>)[^>]*>.*<\/([a-z\-]+)*>/;
+    const singleLineElRegex = /^\s*<([a-z0-9]+)(?![^>]*\/>)[^>]*>.*<\/([a-z0-9\-]+)*>/;
     const lines = tmpl.split('\n');
     const formatted = [];
     let indent = '';
-    for (let line of lines) {
-      if (line.trim() === '') {
-        // skip blank lines
+    for (let [index, line] of lines.entries()) {
+      if (line.trim() === '' && index !== 0 && index !== lines.length - 1) {
+        // skip blank lines except if it's the first line or last line
         continue;
       }
       if ((closeBlockRegex.test(line) ||
diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts
