Skip to content

Commit

Permalink
fix(migrations): avoid conflicts with some greek letters in control f…
Browse files Browse the repository at this point in the history
…low migration (#55113)

The control flow migration was using a couple of Greek letters as placeholders. This ended up conflicting with templates authored in Greek.

These changes use a more obscure placeholder to make conflicts less likely. It also moves the placeholder generation to a centralized function so it's easier to make changes if we decide to update the pattern again.

Fixes #55085.

PR Close #55113
  • Loading branch information
crisbeto authored and atscott committed Mar 29, 2024
1 parent fc03413 commit 949dec2
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 15 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import {visitAll} from '@angular/compiler';

import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
import {calculateNesting, getMainBlock, getOriginals, getPlaceholder, hasLineBreaks, parseTemplate, PlaceholderKind, reduceNestingOffset} from './util';

export const ngfor = '*ngFor';
export const nakedngfor = 'ngFor';
Expand Down Expand Up @@ -112,9 +112,8 @@ function migrateStandardNgFor(etm: ElementToMigrate, tmpl: string, offset: numbe
}
// template
if (part.startsWith('template:')) {
// this generates a special template placeholder just for this use case
// which has a φ at the end instead of the standard δ in other placeholders
tmplPlaceholder = ${part.split(':')[1].trim()}φ`;
// use an alternate placeholder here to avoid conflicts
tmplPlaceholder = getPlaceholder(part.split(':')[1].trim(), PlaceholderKind.Alternate);
}
// aliases
// declared with `let myIndex = index`
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
import {visitAll} from '@angular/compiler';

import {ElementCollector, ElementToMigrate, endMarker, MigrateError, Result, startMarker} from './types';
import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
import {calculateNesting, getMainBlock, getOriginals, getPlaceholder, hasLineBreaks, parseTemplate, PlaceholderKind, reduceNestingOffset} from './util';

export const ngif = '*ngIf';
export const boundngif = '[ngIf]';
Expand Down Expand Up @@ -133,7 +133,7 @@ function buildStandardIfElseBlock(
.replace(' as ', '; as ')
// replace 'let' with 'as' whatever spaces are between ; and 'let'
.replace(/;\s*let/g, '; as');
const elsePlaceholder = ${etm.getTemplateName(elseString)}δ`;
const elsePlaceholder = getPlaceholder(etm.getTemplateName(elseString));
return buildIfElseBlock(etm, tmpl, condition, elsePlaceholder, offset);
}

Expand All @@ -153,9 +153,9 @@ function buildBoundIfElseBlock(etm: ElementToMigrate, tmpl: string, offset: numb
} else if (aliases.length === 1) {
condition += `; as ${aliases[0]}`;
}
const elsePlaceholder = ${etm.elseAttr!.value.trim()}δ`;
const elsePlaceholder = getPlaceholder(etm.elseAttr!.value.trim());
if (etm.thenAttr !== undefined) {
const thenPlaceholder = ${etm.thenAttr!.value.trim()}δ`;
const thenPlaceholder = getPlaceholder(etm.thenAttr!.value.trim());
return buildIfThenElseBlock(etm, tmpl, condition, thenPlaceholder, elsePlaceholder, offset);
}
return buildIfElseBlock(etm, tmpl, condition, elsePlaceholder, offset);
Expand Down Expand Up @@ -194,8 +194,8 @@ function buildStandardIfThenElseBlock(
.replace(' as ', '; as ')
// replace 'let' with 'as' whatever spaces are between ; and 'let'
.replace(/;\s*let/g, '; as');
const thenPlaceholder = ${etm.getTemplateName(thenString, elseString)}δ`;
const elsePlaceholder = ${etm.getTemplateName(elseString)}δ`;
const thenPlaceholder = getPlaceholder(etm.getTemplateName(thenString, elseString));
const elsePlaceholder = getPlaceholder(etm.getTemplateName(elseString));
return buildIfThenElseBlock(etm, tmpl, condition, thenPlaceholder, elsePlaceholder, offset);
}

Expand All @@ -206,7 +206,7 @@ function buildStandardIfThenBlock(
.replace(' as ', '; as ')
// replace 'let' with 'as' whatever spaces are between ; and 'let'
.replace(/;\s*let/g, '; as');
const thenPlaceholder = ${etm.getTemplateName(thenString)}δ`;
const thenPlaceholder = getPlaceholder(etm.getTemplateName(thenString));
return buildIfThenBlock(etm, tmpl, condition, thenPlaceholder, offset);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,21 @@ function isChildOf(parent: Element, el: Element): boolean {
parent.sourceSpan.end.offset > el.sourceSpan.end.offset;
}

/** Possible placeholders that can be generated by `getPlaceholder`. */
export enum PlaceholderKind {
Default,
Alternate,
}

/**
* Wraps a string in a placeholder that makes it easier to identify during replacement operations.
*/
export function getPlaceholder(
value: string, kind: PlaceholderKind = PlaceholderKind.Default): string {
const name = `<<<ɵɵngControlFlowMigration_${kind}ɵɵ>>>`;
return `___${name}${value}${name}___`;
}

/**
* calculates the level of nesting of the items in the collector
*/
Expand Down Expand Up @@ -387,8 +402,8 @@ export function processNgTemplates(template: string): {migrated: string, err: Er

// swap placeholders and remove
for (const [name, t] of templates) {
const replaceRegex = new RegExp(${name.slice(1)}\\δ`, 'g');
const forRegex = new RegExp(${name.slice(1)}\\φ`, 'g');
const replaceRegex = new RegExp(getPlaceholder(name.slice(1)), 'g');
const forRegex = new RegExp(getPlaceholder(name.slice(1), PlaceholderKind.Alternate), 'g');
const forMatches = [...template.matchAll(forRegex)];
const matches = [...forMatches, ...template.matchAll(replaceRegex)];
let safeToRemove = true;
Expand Down Expand Up @@ -429,11 +444,15 @@ export function processNgTemplates(template: string): {migrated: string, err: Er
}

function replaceRemainingPlaceholders(template: string): string {
const replaceRegex = new RegExp(`θ.*δ`, 'g');
const pattern = '.*';
const placeholderPattern = getPlaceholder(pattern);
const replaceRegex = new RegExp(placeholderPattern, 'g');
const [placeholderStart, placeholderEnd] = placeholderPattern.split(pattern);
const placeholders = [...template.matchAll(replaceRegex)];
for (let ph of placeholders) {
const placeholder = ph[0];
const name = placeholder.slice(1, placeholder.length - 1);
const name =
placeholder.slice(placeholderStart.length, placeholder.length - placeholderEnd.length);
template =
template.replace(placeholder, `<ng-template [ngTemplateOutlet]="${name}"></ng-template>`);
}
Expand Down
24 changes: 24 additions & 0 deletions packages/core/schematics/test/control_flow_migration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4364,6 +4364,30 @@ describe('control flow migration', () => {
`}`,
].join('\n'));
});

it('should migrate a template using the θδ characters', async () => {
writeFile('/comp.ts', `
import {Component} from '@angular/core';
import {NgIf} from '@angular/common';
@Component({
selector: 'declare-comp',
templateUrl: './comp.html',
standalone: true,
imports: [NgIf],
})
class DeclareComp {
show = false;
}
`);

writeFile('/comp.html', `<div *ngIf="show">Some greek characters: θδ!</div>`);

await runMigration();

expect(tree.readContent('/comp.html'))
.toBe('@if (show) {<div>Some greek characters: θδ!</div>}');
});
});

describe('formatting', () => {
Expand Down

0 comments on commit 949dec2

Please sign in to comment.