Skip to content

Commit

Permalink
fix(migrations): tweaks to formatting in control flow migration
Browse files Browse the repository at this point in the history 
This addresses a few minor formatting issues with the control flow migration.

fixes: #53017
  • Loading branch information
@thePunderWoman
thePunderWoman committed Nov 20, 2023
1 parent 406049b commit 1aa9260
Show file tree
Hide file tree
Showing 7 changed files with 130 additions and 32 deletions.
8 changes: 5 additions & 3 deletions packages/core/schematics/ng-generate/control-flow-migration/cases.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,11 @@ 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;
Expand Down Expand Up @@ -73,7 +73,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 {
Expand Down
9 changes: 6 additions & 3 deletions packages/core/schematics/ng-generate/control-flow-migration/fors.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
9 changes: 6 additions & 3 deletions packages/core/schematics/ng-generate/control-flow-migration/ifs.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
4 changes: 3 additions & 1 deletion packages/core/schematics/ng-generate/control-flow-migration/migration.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
9 changes: 6 additions & 3 deletions packages/core/schematics/ng-generate/control-flow-migration/switches.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down
29 changes: 21 additions & 8 deletions packages/core/schematics/ng-generate/control-flow-migration/util.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,26 @@ 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();
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());
Expand Down Expand Up @@ -326,7 +339,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;
Expand Down Expand Up @@ -423,29 +436,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) ||
Expand Down
94 changes: 83 additions & 11 deletions packages/core/schematics/test/control_flow_migration_spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ describe('control flow migration', () => {
` } @else {`,
` <p>Stuff</p>`,
` }`,
`</div>`,
`</div>\n`,
].join('\n'));
});

Expand Down Expand Up @@ -392,7 +392,7 @@ describe('control flow migration', () => {
` } @else {`,
` <p>Stuff</p>`,
` }`,
`</div>`,
`</div>\n`,
].join('\n'));
});

Expand Down Expand Up @@ -582,7 +582,7 @@ describe('control flow migration', () => {
` } @else {`,
` <ng-container i18n="@@somethingElse"><p>other</p></ng-container>`,
` }`,
`</div>`,
`</div>\n`,
].join('\n'));
});

Expand Down Expand Up @@ -616,7 +616,7 @@ describe('control flow migration', () => {
` } @else {`,
` <ng-container i18n="@@somethingElse"><p>other</p></ng-container>`,
` }`,
`</div>`,
`</div>\n`,
].join('\n'));
});

Expand Down Expand Up @@ -3081,7 +3081,7 @@ describe('control flow migration', () => {
` height="2rem"`,
` width="6rem" />`,
` }`,
`</div>`,
`</div>\n`,
].join('\n');

expect(content).toBe(result);
Expand Down Expand Up @@ -3122,7 +3122,7 @@ describe('control flow migration', () => {
` <div class="test"></div>`,
` }`,
` }`,
`</div>`,
`</div>\n`,
].join('\n');

expect(actual).toBe(expected);
Expand Down Expand Up @@ -3165,7 +3165,7 @@ describe('control flow migration', () => {
` }`,
` </ng-container>`,
` }`,
`</div>`,
`</div>\n`,
].join('\n');

expect(actual).toBe(expected);
Expand Down Expand Up @@ -3208,7 +3208,7 @@ describe('control flow migration', () => {
` } @else {`,
` <p>No Permissions</p>`,
` }`,
`</div>`,
`</div>\n`,
].join('\n');

expect(actual).toBe(expected);
Expand All @@ -3228,8 +3228,10 @@ describe('control flow migration', () => {
`);

writeFile('/comp.html', [
`<div *ngIf="true">changed</div>`,
`<div>`,
`@if (stuff) {`,
`<h2>Title</h2>`,
`<p>Stuff</p>`,
`} @else if (things) {`,
`<p>Things</p>`,
Expand All @@ -3243,8 +3245,12 @@ describe('control flow migration', () => {
const actual = tree.readContent('/comp.html');

const expected = [
`@if (true) {`,
` <div>changed</div>`,
`}`,
`<div>`,
` @if (stuff) {`,
` <h2>Title</h2>`,
` <p>Stuff</p>`,
` } @else if (things) {`,
` <p>Things</p>`,
Expand All @@ -3270,28 +3276,32 @@ describe('control flow migration', () => {
`);

writeFile('/comp.html', [
`<div *ngIf="true">changed</div>`,
`<div>`,
`@if (stuff) {`,
`<img src="path.png" alt="stuff" />`,
`} @else {`,
`<img src="path.png"`,
`alt="stuff" />`,
`}`,
`</div>`,
`</div>\n`,
].join('\n'));

await runMigration();
const actual = tree.readContent('/comp.html');

const expected = [
`@if (true) {`,
` <div>changed</div>`,
`}`,
`<div>`,
` @if (stuff) {`,
` <img src="path.png" alt="stuff" />`,
` } @else {`,
` <img src="path.png"`,
` alt="stuff" />`,
` }`,
`</div>`,
`</div>\n`,
].join('\n');

expect(actual).toBe(expected);
Expand Down Expand Up @@ -3362,6 +3372,68 @@ describe('control flow migration', () => {
expect(actual).toBe(expected);
});

it('should not remove common module imports post migration if other items used', async () => {
writeFile('/comp.ts', [
`import {CommonModule} from '@angular/common';`,
`import {Component} from '@angular/core';\n`,
`@Component({`,
` imports: [NgIf, DatePipe],`,
` template: \`<div><span *ngIf="toggle">{{ d | date }}</span></div>\``,
`})`,
`class Comp {`,
` toggle = false;`,
`}`,
].join('\n'));

await runMigration();
const actual = tree.readContent('/comp.ts');
const expected = [
`import {CommonModule} from '@angular/common';`,
`import {Component} from '@angular/core';\n`,
`@Component({`,
` imports: [DatePipe],`,
` template: \`<div>@if (toggle) {<span>{{ d | date }}</span>}</div>\``,
`})`,
`class Comp {`,
` toggle = false;`,
`}`,
].join('\n');

expect(actual).toBe(expected);
});

it('should not duplicate comments post migration if other items used', async () => {
writeFile('/comp.ts', [
`// comment here`,
`import {NgIf, CommonModule} from '@angular/common';`,
`import {Component} from '@angular/core';\n`,
`@Component({`,
` imports: [NgIf, DatePipe],`,
` template: \`<div><span *ngIf="toggle">{{ d | date }}</span></div>\``,
`})`,
`class Comp {`,
` toggle = false;`,
`}`,
].join('\n'));

await runMigration();
const actual = tree.readContent('/comp.ts');
const expected = [
`// comment here`,
`import { CommonModule } from '@angular/common';`,
`import {Component} from '@angular/core';\n`,
`@Component({`,
` imports: [DatePipe],`,
` template: \`<div>@if (toggle) {<span>{{ d | date }}</span>}</div>\``,
`})`,
`class Comp {`,
` toggle = false;`,
`}`,
].join('\n');

expect(actual).toBe(expected);
});

it('should leave non-cf common module imports post migration', async () => {
writeFile('/comp.ts', [
`import {Component} from '@angular/core';`,
Expand Down Expand Up @@ -3438,7 +3510,7 @@ describe('control flow migration', () => {
const actual = tree.readContent('/comp.ts');
const expected = [
`import {Component} from '@angular/core';`,
`import { CommonModule } from '@angular/common';\n`,
`import {CommonModule} from '@angular/common';\n`,
`@Component({`,
` imports: [CommonModule],`,
` template: \`<div>@if (toggle) {<span>{{ shrug | lowercase }}</span>}</div>\``,
Expand Down

0 comments on commit 1aa9260

Please sign in to comment.