Skip to content

Commit b75aca1

Browse files
thePunderWomanalxhub
thePunderWoman
authored and
alxhub
committed
fix(migrations): CF migration only remove newlines of changed template content (#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: #53494 PR Close #53508
1 parent d232ea1 commit b75aca1

File tree

8 files changed

+100
-27
lines changed

8 files changed

+100
-27
lines changed

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import {visitAll} from '@angular/compiler';
1010

11-
import {ElementCollector, ElementToMigrate, MigrateError, Result} from './types';
11+
import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
1212
import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
1313

1414
export const boundcase = '[ngSwitchCase]';
@@ -88,8 +88,9 @@ function migrateNgSwitchCase(etm: ElementToMigrate, tmpl: string, offset: number
8888
const originals = getOriginals(etm, tmpl, offset);
8989

9090
const {start, middle, end} = getMainBlock(etm, tmpl, offset);
91-
const startBlock = `${leadingSpace}@case (${condition}) {${leadingSpace}${lbString}${start}`;
92-
const endBlock = `${end}${lbString}${leadingSpace}}`;
91+
const startBlock =
92+
`${startMarker}${leadingSpace}@case (${condition}) {${leadingSpace}${lbString}${start}`;
93+
const endBlock = `${end}${lbString}${leadingSpace}}${endMarker}`;
9394

9495
const defaultBlock = startBlock + middle + endBlock;
9596
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
110111
const originals = getOriginals(etm, tmpl, offset);
111112

112113
const {start, middle, end} = getMainBlock(etm, tmpl, offset);
113-
const startBlock = `${leadingSpace}@default {${leadingSpace}${lbString}${start}`;
114-
const endBlock = `${end}${lbString}${leadingSpace}}`;
114+
const startBlock = `${startMarker}${leadingSpace}@default {${leadingSpace}${lbString}${start}`;
115+
const endBlock = `${end}${lbString}${leadingSpace}}${endMarker}`;
115116

116117
const defaultBlock = startBlock + middle + endBlock;
117118
const updatedTmpl = tmpl.slice(0, etm.start(offset)) + defaultBlock + tmpl.slice(etm.end(offset));

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

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import {visitAll} from '@angular/compiler';
1010

11-
import {ElementCollector, ElementToMigrate, MigrateError, Result} from './types';
11+
import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
1212
import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
1313

1414
export const ngfor = '*ngFor';
@@ -149,8 +149,8 @@ function migrateStandardNgFor(etm: ElementToMigrate, tmpl: string, offset: numbe
149149

150150
const aliasStr = (aliases.length > 0) ? `;${aliases.join(';')}` : '';
151151

152-
let startBlock = `@for (${condition}; track ${trackBy}${aliasStr}) {${lbString}`;
153-
let endBlock = `${lbString}}`;
152+
let startBlock = `${startMarker}@for (${condition}; track ${trackBy}${aliasStr}) {${lbString}`;
153+
let endBlock = `${lbString}}${endMarker}`;
154154
let forBlock = '';
155155

156156
if (tmplPlaceholder !== '') {
@@ -196,9 +196,9 @@ function migrateBoundNgFor(etm: ElementToMigrate, tmpl: string, offset: number):
196196
}
197197

198198
const {start, middle, end} = getMainBlock(etm, tmpl, offset);
199-
const startBlock = `@for (${condition}; track ${trackBy}${aliasStr}) {\n${start}`;
199+
const startBlock = `${startMarker}@for (${condition}; track ${trackBy}${aliasStr}) {\n${start}`;
200200

201-
const endBlock = `${end}\n}`;
201+
const endBlock = `${end}\n}${endMarker}`;
202202
const forBlock = startBlock + middle + endBlock;
203203

204204
const updatedTmpl = tmpl.slice(0, etm.start(offset)) + forBlock + tmpl.slice(etm.end(offset));

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

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import {visitAll} from '@angular/compiler';
1010

11-
import {ElementCollector, ElementToMigrate, MigrateError, Result} from './types';
11+
import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
1212
import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
1313

1414
export const ngif = '*ngIf';
@@ -112,8 +112,8 @@ function buildIfBlock(etm: ElementToMigrate, tmpl: string, offset: number): Resu
112112
const originals = getOriginals(etm, tmpl, offset);
113113

114114
const {start, middle, end} = getMainBlock(etm, tmpl, offset);
115-
const startBlock = `@if (${condition}) {${lbString}${start}`;
116-
const endBlock = `${end}${lbString}}`;
115+
const startBlock = `${startMarker}@if (${condition}) {${lbString}${start}`;
116+
const endBlock = `${end}${lbString}}${endMarker}`;
117117

118118
const ifBlock = startBlock + middle + endBlock;
119119
const updatedTmpl = tmpl.slice(0, etm.start(offset)) + ifBlock + tmpl.slice(etm.end(offset));
@@ -169,11 +169,11 @@ function buildIfElseBlock(
169169
const originals = getOriginals(etm, tmpl, offset);
170170

171171
const {start, middle, end} = getMainBlock(etm, tmpl, offset);
172-
const startBlock = `@if (${condition}) {${lbString}${start}`;
172+
const startBlock = `${startMarker}@if (${condition}) {${lbString}${start}`;
173173

174174
const elseBlock = `${end}${lbString}} @else {${lbString}`;
175175

176-
const postBlock = elseBlock + elsePlaceholder + `${lbString}}`;
176+
const postBlock = elseBlock + elsePlaceholder + `${lbString}}${endMarker}`;
177177
const ifElseBlock = startBlock + middle + postBlock;
178178

179179
const tmplStart = tmpl.slice(0, etm.start(offset));
@@ -217,10 +217,10 @@ function buildIfThenElseBlock(
217217

218218
const originals = getOriginals(etm, tmpl, offset);
219219

220-
const startBlock = `@if (${condition}) {${lbString}`;
220+
const startBlock = `${startMarker}@if (${condition}) {${lbString}`;
221221
const elseBlock = `${lbString}} @else {${lbString}`;
222222

223-
const postBlock = thenPlaceholder + elseBlock + elsePlaceholder + `${lbString}}`;
223+
const postBlock = thenPlaceholder + elseBlock + elsePlaceholder + `${lbString}}${endMarker}`;
224224
const ifThenElseBlock = startBlock + postBlock;
225225

226226
const tmplStart = tmpl.slice(0, etm.start(offset));
@@ -243,9 +243,9 @@ function buildIfThenBlock(
243243

244244
const originals = getOriginals(etm, tmpl, offset);
245245

246-
const startBlock = `@if (${condition}) {${lbString}`;
246+
const startBlock = `${startMarker}@if (${condition}) {${lbString}`;
247247

248-
const postBlock = thenPlaceholder + `${lbString}}`;
248+
const postBlock = thenPlaceholder + `${lbString}}${endMarker}`;
249249
const ifThenBlock = startBlock + postBlock;
250250

251251
const tmplStart = tmpl.slice(0, etm.start(offset));

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@ import {migrateCase} from './cases';
1212
import {migrateFor} from './fors';
1313
import {migrateIf} from './ifs';
1414
import {migrateSwitch} from './switches';
15-
import {AnalyzedFile, MigrateError} from './types';
15+
import {AnalyzedFile, endMarker, MigrateError, startMarker} from './types';
1616
import {canRemoveCommonModule, formatTemplate, processNgTemplates, removeImports} from './util';
1717

1818
/**
@@ -39,6 +39,8 @@ export function migrateTemplate(
3939
if (format && changed) {
4040
migrated = formatTemplate(migrated, templateType);
4141
}
42+
const markerRegex = new RegExp(`${startMarker}|${endMarker}`, 'gm');
43+
migrated = migrated.replace(markerRegex, '');
4244
file.removeCommonModule = canRemoveCommonModule(template);
4345
file.canRemoveImports = true;
4446

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

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@
88

99
import {visitAll} from '@angular/compiler';
1010

11-
import {ElementCollector, ElementToMigrate, MigrateError, Result} from './types';
11+
import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
1212
import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
1313

1414
export const ngswitch = '[ngSwitch]';
@@ -72,8 +72,8 @@ function migrateNgSwitch(etm: ElementToMigrate, tmpl: string, offset: number): R
7272
const originals = getOriginals(etm, tmpl, offset);
7373

7474
const {start, middle, end} = getMainBlock(etm, tmpl, offset);
75-
const startBlock = `${start}${lbString}@switch (${condition}) {`;
76-
const endBlock = `}${lbString}${end}`;
75+
const startBlock = `${startMarker}${start}${lbString}@switch (${condition}) {`;
76+
const endBlock = `}${lbString}${end}${endMarker}`;
7777

7878
const switchBlock = startBlock + middle + endBlock;
7979
const updatedTmpl = tmpl.slice(0, etm.start(offset)) + switchBlock + tmpl.slice(etm.end(offset));

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,9 @@ export const boundngifthenelse = '[ngIfThenElse]';
1515
export const boundngifthen = '[ngIfThen]';
1616
export const nakedngfor = 'ngFor';
1717

18+
export const startMarker = '◬';
19+
export const endMarker = '✢';
20+
1821
function allFormsOf(selector: string): string[] {
1922
return [
2023
selector,

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

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,16 @@ import {Attribute, HtmlParser, ParseTreeResult, visitAll} from '@angular/compile
1010
import {dirname, join} from 'path';
1111
import ts from 'typescript';
1212

13-
import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, Template, TemplateCollector} from './types';
13+
import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, endMarker, startMarker, Template, TemplateCollector} from './types';
1414

1515
const importRemovals = [
1616
'NgIf', 'NgIfElse', 'NgIfThenElse', 'NgFor', 'NgForOf', 'NgForTrackBy', 'NgSwitch',
1717
'NgSwitchCase', 'NgSwitchDefault'
1818
];
1919
const importWithCommonRemovals = [...importRemovals, 'CommonModule'];
20+
const startMarkerRegex = new RegExp(startMarker, 'gm');
21+
const endMarkerRegex = new RegExp(endMarker, 'gm');
22+
const replaceMarkerRegex = new RegExp(`${startMarker}|${endMarker}`, 'gm');
2023

2124
/**
2225
* 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
348351
}
349352
// the +1 accounts for the t.count's counting of the original template
350353
if (t.count === matches.length + 1 && safeToRemove) {
351-
template = template.replace(t.contents, '');
354+
template = template.replace(t.contents, `${startMarker}${endMarker}`);
352355
}
353356
// templates may have changed structure from nested replaced templates
354357
// so we need to reprocess them before the next loop.
@@ -474,6 +477,8 @@ export function getMainBlock(etm: ElementToMigrate, tmpl: string, offset: number
474477
if (etm.hasChildren()) {
475478
const {childStart, childEnd} = etm.getChildSpan(offset);
476479
middle = tmpl.slice(childStart, childEnd);
480+
} else {
481+
middle = startMarker + endMarker;
477482
}
478483
return {start: '', middle, end: ''};
479484
} else if (isI18nTemplate(etm, i18nAttr)) {
@@ -512,6 +517,13 @@ export function getMainBlock(etm: ElementToMigrate, tmpl: string, offset: number
512517

513518
const selfClosingList = 'input|br|img|base|wbr|area|col|embed|hr|link|meta|param|source|track';
514519

520+
function isInMigratedBlock(line: string, isCurrentlyInMigratedBlock: boolean) {
521+
return line.match(startMarkerRegex) &&
522+
(!line.match(endMarkerRegex) ||
523+
line.lastIndexOf(startMarker) > line.lastIndexOf(endMarker)) ||
524+
(isCurrentlyInMigratedBlock && !line.match(endMarkerRegex));
525+
}
526+
515527
/**
516528
* re-indents all the lines in the template properly post migration
517529
*/
@@ -561,8 +573,19 @@ export function formatTemplate(tmpl: string, templateType: string): string {
561573
let indent = '';
562574
// the pre-existing indent in an inline template that we'd like to preserve
563575
let mindent = '';
576+
let depth = 0;
577+
let inMigratedBlock = false;
564578
for (let [index, line] of lines.entries()) {
565-
if (line.trim() === '' && index !== 0 && index !== lines.length - 1) {
579+
depth +=
580+
[...line.matchAll(startMarkerRegex)].length - [...line.matchAll(endMarkerRegex)].length;
581+
inMigratedBlock = depth > 0;
582+
let lineWasMigrated = false;
583+
if (line.match(replaceMarkerRegex)) {
584+
line = line.replace(replaceMarkerRegex, '');
585+
lineWasMigrated = true;
586+
}
587+
if ((line.trim() === '' && index !== 0 && index !== lines.length - 1) &&
588+
(inMigratedBlock || lineWasMigrated)) {
566589
// skip blank lines except if it's the first line or last line
567590
// this preserves leading and trailing spaces if they are already present
568591
continue;
@@ -584,7 +607,7 @@ export function formatTemplate(tmpl: string, templateType: string): string {
584607
indent = indent.slice(2);
585608
}
586609

587-
formatted.push(mindent + indent + line.trim());
610+
formatted.push(mindent + (line.trim() !== '' ? indent : '') + line.trim());
588611

589612
// this matches any self closing element that actually has a />
590613
if (closeMultiLineElRegex.test(line)) {

‎packages/core/schematics/test/control_flow_migration_spec.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4391,6 +4391,50 @@ describe('control flow migration', () => {
43914391
expect(actual).toBe(expected);
43924392
});
43934393

4394+
it('should remove empty lines only in parts of template that were changed', async () => {
4395+
writeFile('/comp.ts', `
4396+
import {Component} from '@angular/core';
4397+
import {NgIf} from '@angular/common';
4398+
4399+
@Component({
4400+
selector: 'declare-comp',
4401+
templateUrl: 'comp.html',
4402+
})
4403+
class DeclareComp {}
4404+
`);
4405+
4406+
writeFile('/comp.html', [
4407+
`<div>header</div>\n`,
4408+
`<span>header</span>\n\n\n`,
4409+
`<div *ngIf="true">changed</div>`,
4410+
`<div>\n`,
4411+
` <ul>`,
4412+
` <li *ngFor="let item of items">{{ item }}</li>`,
4413+
` </ul>`,
4414+
`</div>\n\n`,
4415+
].join('\n'));
4416+
4417+
await runMigration();
4418+
const actual = tree.readContent('/comp.html');
4419+
4420+
const expected = [
4421+
`<div>header</div>\n`,
4422+
`<span>header</span>\n\n\n`,
4423+
`@if (true) {`,
4424+
` <div>changed</div>`,
4425+
`}`,
4426+
`<div>\n`,
4427+
` <ul>`,
4428+
` @for (item of items; track item) {`,
4429+
` <li>{{ item }}</li>`,
4430+
` }`,
4431+
` </ul>`,
4432+
`</div>\n\n`,
4433+
].join('\n');
4434+
4435+
expect(actual).toBe(expected);
4436+
});
4437+
43944438
it('should reformat properly with if else and mixed content', async () => {
43954439
writeFile('/comp.ts', `
43964440
import {Component} from '@angular/core';

0 commit comments

Comments
 (0)