fix(migrations): CF migration only remove newlines of changed templat…

…e content (angular#53508)

The formatting logic would eliminate all newlines in updated template code. This adds start and end markers for tracking when the formatter is in a block of template code that changed or not. It should leave behind any newlines that are outside of a migrated section.

fixes: angular#53494

PR Close angular#53508

packages/core/schematics/ng-generate/control-flow-migration/cases.ts

@@ -8,7 +8,7 @@
 
 import {visitAll} from '@angular/compiler';
 
-import {ElementCollector, ElementToMigrate, MigrateError, Result} from './types';
+import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
 import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
 
 export const boundcase = '[ngSwitchCase]';
@@ -88,8 +88,9 @@ function migrateNgSwitchCase(etm: ElementToMigrate, tmpl: string, offset: number
   const originals = getOriginals(etm, tmpl, offset);
 
   const {start, middle, end} = getMainBlock(etm, tmpl, offset);
-  const startBlock = `${leadingSpace}@case (${condition}) {${leadingSpace}${lbString}${start}`;
-  const endBlock = `${end}${lbString}${leadingSpace}}`;
+  const startBlock =
+      `${startMarker}${leadingSpace}@case (${condition}) {${leadingSpace}${lbString}${start}`;
+  const endBlock = `${end}${lbString}${leadingSpace}}${endMarker}`;
 
   const defaultBlock = startBlock + middle + endBlock;
   const updatedTmpl = tmpl.slice(0, etm.start(offset)) + defaultBlock + tmpl.slice(etm.end(offset));
@@ -110,8 +111,8 @@ function migrateNgSwitchDefault(etm: ElementToMigrate, tmpl: string, offset: num
   const originals = getOriginals(etm, tmpl, offset);
 
   const {start, middle, end} = getMainBlock(etm, tmpl, offset);
-  const startBlock = `${leadingSpace}@default {${leadingSpace}${lbString}${start}`;
-  const endBlock = `${end}${lbString}${leadingSpace}}`;
+  const startBlock = `${startMarker}${leadingSpace}@default {${leadingSpace}${lbString}${start}`;
+  const endBlock = `${end}${lbString}${leadingSpace}}${endMarker}`;
 
   const defaultBlock = startBlock + middle + endBlock;
   const updatedTmpl = tmpl.slice(0, etm.start(offset)) + defaultBlock + tmpl.slice(etm.end(offset));

packages/core/schematics/ng-generate/control-flow-migration/fors.ts

@@ -8,7 +8,7 @@
 
 import {visitAll} from '@angular/compiler';
 
-import {ElementCollector, ElementToMigrate, MigrateError, Result} from './types';
+import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
 import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
 
 export const ngfor = '*ngFor';
@@ -149,8 +149,8 @@ function migrateStandardNgFor(etm: ElementToMigrate, tmpl: string, offset: numbe
 
   const aliasStr = (aliases.length > 0) ? `;${aliases.join(';')}` : '';
 
-  let startBlock = `@for (${condition}; track ${trackBy}${aliasStr}) {${lbString}`;
-  let endBlock = `${lbString}}`;
+  let startBlock = `${startMarker}@for (${condition}; track ${trackBy}${aliasStr}) {${lbString}`;
+  let endBlock = `${lbString}}${endMarker}`;
   let forBlock = '';
 
   if (tmplPlaceholder !== '') {
@@ -196,9 +196,9 @@ function migrateBoundNgFor(etm: ElementToMigrate, tmpl: string, offset: number):
   }
 
   const {start, middle, end} = getMainBlock(etm, tmpl, offset);
-  const startBlock = `@for (${condition}; track ${trackBy}${aliasStr}) {\n${start}`;
+  const startBlock = `${startMarker}@for (${condition}; track ${trackBy}${aliasStr}) {\n${start}`;
 
-  const endBlock = `${end}\n}`;
+  const endBlock = `${end}\n}${endMarker}`;
   const forBlock = startBlock + middle + endBlock;
 
   const updatedTmpl = tmpl.slice(0, etm.start(offset)) + forBlock + tmpl.slice(etm.end(offset));

packages/core/schematics/ng-generate/control-flow-migration/ifs.ts

@@ -8,7 +8,7 @@
 
 import {visitAll} from '@angular/compiler';
 
-import {ElementCollector, ElementToMigrate, MigrateError, Result} from './types';
+import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
 import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
 
 export const ngif = '*ngIf';
@@ -112,8 +112,8 @@ function buildIfBlock(etm: ElementToMigrate, tmpl: string, offset: number): Resu
   const originals = getOriginals(etm, tmpl, offset);
 
   const {start, middle, end} = getMainBlock(etm, tmpl, offset);
-  const startBlock = `@if (${condition}) {${lbString}${start}`;
-  const endBlock = `${end}${lbString}}`;
+  const startBlock = `${startMarker}@if (${condition}) {${lbString}${start}`;
+  const endBlock = `${end}${lbString}}${endMarker}`;
 
   const ifBlock = startBlock + middle + endBlock;
   const updatedTmpl = tmpl.slice(0, etm.start(offset)) + ifBlock + tmpl.slice(etm.end(offset));
@@ -169,11 +169,11 @@ function buildIfElseBlock(
   const originals = getOriginals(etm, tmpl, offset);
 
   const {start, middle, end} = getMainBlock(etm, tmpl, offset);
-  const startBlock = `@if (${condition}) {${lbString}${start}`;
+  const startBlock = `${startMarker}@if (${condition}) {${lbString}${start}`;
 
   const elseBlock = `${end}${lbString}} @else {${lbString}`;
 
-  const postBlock = elseBlock + elsePlaceholder + `${lbString}}`;
+  const postBlock = elseBlock + elsePlaceholder + `${lbString}}${endMarker}`;
   const ifElseBlock = startBlock + middle + postBlock;
 
   const tmplStart = tmpl.slice(0, etm.start(offset));
@@ -217,10 +217,10 @@ function buildIfThenElseBlock(
 
   const originals = getOriginals(etm, tmpl, offset);
 
-  const startBlock = `@if (${condition}) {${lbString}`;
+  const startBlock = `${startMarker}@if (${condition}) {${lbString}`;
   const elseBlock = `${lbString}} @else {${lbString}`;
 
-  const postBlock = thenPlaceholder + elseBlock + elsePlaceholder + `${lbString}}`;
+  const postBlock = thenPlaceholder + elseBlock + elsePlaceholder + `${lbString}}${endMarker}`;
   const ifThenElseBlock = startBlock + postBlock;
 
   const tmplStart = tmpl.slice(0, etm.start(offset));
@@ -243,9 +243,9 @@ function buildIfThenBlock(
 
   const originals = getOriginals(etm, tmpl, offset);
 
-  const startBlock = `@if (${condition}) {${lbString}`;
+  const startBlock = `${startMarker}@if (${condition}) {${lbString}`;
 
-  const postBlock = thenPlaceholder + `${lbString}}`;
+  const postBlock = thenPlaceholder + `${lbString}}${endMarker}`;
   const ifThenBlock = startBlock + postBlock;
 
   const tmplStart = tmpl.slice(0, etm.start(offset));

packages/core/schematics/ng-generate/control-flow-migration/migration.ts

@@ -12,7 +12,7 @@ import {migrateCase} from './cases';
 import {migrateFor} from './fors';
 import {migrateIf} from './ifs';
 import {migrateSwitch} from './switches';
-import {AnalyzedFile, MigrateError} from './types';
+import {AnalyzedFile, endMarker, MigrateError, startMarker} from './types';
 import {canRemoveCommonModule, formatTemplate, processNgTemplates, removeImports} from './util';
 
 /**
@@ -39,6 +39,8 @@ export function migrateTemplate(
     if (format && changed) {
       migrated = formatTemplate(migrated, templateType);
     }
+    const markerRegex = new RegExp(`${startMarker}|${endMarker}`, 'gm');
+    migrated = migrated.replace(markerRegex, '');
     file.removeCommonModule = canRemoveCommonModule(template);
     file.canRemoveImports = true;
 

packages/core/schematics/ng-generate/control-flow-migration/switches.ts

@@ -8,7 +8,7 @@
 
 import {visitAll} from '@angular/compiler';
 
-import {ElementCollector, ElementToMigrate, MigrateError, Result} from './types';
+import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
 import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
 
 export const ngswitch = '[ngSwitch]';
@@ -72,8 +72,8 @@ function migrateNgSwitch(etm: ElementToMigrate, tmpl: string, offset: number): R
   const originals = getOriginals(etm, tmpl, offset);
 
   const {start, middle, end} = getMainBlock(etm, tmpl, offset);
-  const startBlock = `${start}${lbString}@switch (${condition}) {`;
-  const endBlock = `}${lbString}${end}`;
+  const startBlock = `${startMarker}${start}${lbString}@switch (${condition}) {`;
+  const endBlock = `}${lbString}${end}${endMarker}`;
 
   const switchBlock = startBlock + middle + endBlock;
   const updatedTmpl = tmpl.slice(0, etm.start(offset)) + switchBlock + tmpl.slice(etm.end(offset));

packages/core/schematics/ng-generate/control-flow-migration/types.ts

@@ -15,6 +15,9 @@ export const boundngifthenelse = '[ngIfThenElse]';
 export const boundngifthen = '[ngIfThen]';
 export const nakedngfor = 'ngFor';
 
+export const startMarker = '◬';
+export const endMarker = '✢';
+
 function allFormsOf(selector: string): string[] {
   return [
     selector,

packages/core/schematics/ng-generate/control-flow-migration/util.ts

@@ -10,13 +10,16 @@ import {Attribute, HtmlParser, ParseTreeResult, visitAll} from '@angular/compile
 import {dirname, join} from 'path';
 import ts from 'typescript';
 
-import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, Template, TemplateCollector} from './types';
+import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, endMarker, startMarker, Template, TemplateCollector} from './types';
 
 const importRemovals = [
   'NgIf', 'NgIfElse', 'NgIfThenElse', 'NgFor', 'NgForOf', 'NgForTrackBy', 'NgSwitch',
   'NgSwitchCase', 'NgSwitchDefault'
 ];
 const importWithCommonRemovals = [...importRemovals, 'CommonModule'];
+const startMarkerRegex = new RegExp(startMarker, 'gm');
+const endMarkerRegex = new RegExp(endMarker, 'gm');
+const replaceMarkerRegex = new RegExp(`${startMarker}|${endMarker}`, 'gm');
 
 /**
  * Analyzes a source file to find file that need to be migrated and the text ranges within them.
@@ -348,7 +351,7 @@ export function processNgTemplates(template: string): {migrated: string, err: Er
         }
         // the +1 accounts for the t.count's counting of the original template
         if (t.count === matches.length + 1 && safeToRemove) {
-          template = template.replace(t.contents, '');
+          template = template.replace(t.contents, `${startMarker}${endMarker}`);
         }
         // templates may have changed structure from nested replaced templates
         // so we need to reprocess them before the next loop.
@@ -474,6 +477,8 @@ export function getMainBlock(etm: ElementToMigrate, tmpl: string, offset: number
     if (etm.hasChildren()) {
       const {childStart, childEnd} = etm.getChildSpan(offset);
       middle = tmpl.slice(childStart, childEnd);
+    } else {
+      middle = startMarker + endMarker;
     }
     return {start: '', middle, end: ''};
   } else if (isI18nTemplate(etm, i18nAttr)) {
@@ -512,6 +517,13 @@ export function getMainBlock(etm: ElementToMigrate, tmpl: string, offset: number
 
 const selfClosingList = 'input|br|img|base|wbr|area|col|embed|hr|link|meta|param|source|track';
 
+function isInMigratedBlock(line: string, isCurrentlyInMigratedBlock: boolean) {
+  return line.match(startMarkerRegex) &&
+      (!line.match(endMarkerRegex) ||
+       line.lastIndexOf(startMarker) > line.lastIndexOf(endMarker)) ||
+      (isCurrentlyInMigratedBlock && !line.match(endMarkerRegex));
+}
+
 /**
  * re-indents all the lines in the template properly post migration
  */
@@ -561,8 +573,19 @@ export function formatTemplate(tmpl: string, templateType: string): string {
     let indent = '';
     // the pre-existing indent in an inline template that we'd like to preserve
     let mindent = '';
+    let depth = 0;
+    let inMigratedBlock = false;
     for (let [index, line] of lines.entries()) {
-      if (line.trim() === '' && index !== 0 && index !== lines.length - 1) {
+      depth +=
+          [...line.matchAll(startMarkerRegex)].length - [...line.matchAll(endMarkerRegex)].length;
+      inMigratedBlock = depth > 0;
+      let lineWasMigrated = false;
+      if (line.match(replaceMarkerRegex)) {
+        line = line.replace(replaceMarkerRegex, '');
+        lineWasMigrated = true;
+      }
+      if ((line.trim() === '' && index !== 0 && index !== lines.length - 1) &&
+          (inMigratedBlock || lineWasMigrated)) {
         // skip blank lines except if it's the first line or last line
         // this preserves leading and trailing spaces if they are already present
         continue;
@@ -584,7 +607,7 @@ export function formatTemplate(tmpl: string, templateType: string): string {
         indent = indent.slice(2);
       }
 
-      formatted.push(mindent + indent + line.trim());
+      formatted.push(mindent + (line.trim() !== '' ? indent : '') + line.trim());
 
       // this matches any self closing element that actually has a />
       if (closeMultiLineElRegex.test(line)) {

packages/core/schematics/test/control_flow_migration_spec.ts

@@ -4391,6 +4391,50 @@ describe('control flow migration', () => {
       expect(actual).toBe(expected);
     });
 
+    it('should remove empty lines only in parts of template that were changed', async () => {
+      writeFile('/comp.ts', `
+        import {Component} from '@angular/core';
+        import {NgIf} from '@angular/common';
+
+        @Component({
+          selector: 'declare-comp',
+          templateUrl: 'comp.html',
+        })
+        class DeclareComp {}
+      `);
+
+      writeFile('/comp.html', [
+        `<div>header</div>\n`,
+        `<span>header</span>\n\n\n`,
+        `<div *ngIf="true">changed</div>`,
+        `<div>\n`,
+        `  <ul>`,
+        `    <li *ngFor="let item of items">{{ item }}</li>`,
+        `  </ul>`,
+        `</div>\n\n`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.html');
+
+      const expected = [
+        `<div>header</div>\n`,
+        `<span>header</span>\n\n\n`,
+        `@if (true) {`,
+        `  <div>changed</div>`,
+        `}`,
+        `<div>\n`,
+        `  <ul>`,
+        `    @for (item of items; track item) {`,
+        `      <li>{{ item }}</li>`,
+        `    }`,
+        `  </ul>`,
+        `</div>\n\n`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
+
     it('should reformat properly with if else and mixed content', async () => {
       writeFile('/comp.ts', `
         import {Component} from '@angular/core';