Skip to content

Commit

Permalink
fix(migrations): Update CF migration to skip templates with duplicate…
Browse files Browse the repository at this point in the history
… ng-template names (#53204)

This adds a message to the console and skips any templates that detect duplicate ng-template names in the same component.
fixes: #53169

PR Close #53204
  • Loading branch information
thePunderWoman authored and pkozlowski-opensource committed Nov 28, 2023
1 parent dbd6f38 commit 0fcef65
Show file tree
Hide file tree
Showing 4 changed files with 73 additions and 31 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@ export function migrateTemplate(
const forResult = migrateFor(ifResult.migrated);
const switchResult = migrateSwitch(forResult.migrated);
const caseResult = migrateCase(switchResult.migrated);
migrated = processNgTemplates(caseResult.migrated);
const templateResult = processNgTemplates(caseResult.migrated);
if (templateResult.err !== undefined) {
return {migrated: template, errors: [{type: 'template', error: templateResult.err}]};
}
migrated = templateResult.migrated;
const changed =
ifResult.changed || forResult.changed || switchResult.changed || caseResult.changed;
if (format && changed) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -338,9 +338,13 @@ export class TemplateCollector extends RecursiveVisitor {
templateAttr = attr;
}
}
if (templateAttr !== null) {
this.elements.push(new ElementToMigrate(el, templateAttr));
if (templateAttr !== null && !this.templates.has(templateAttr.name)) {
this.templates.set(templateAttr.name, new Template(el, templateAttr.name, i18n));
this.elements.push(new ElementToMigrate(el, templateAttr));
} else if (templateAttr !== null) {
throw new Error(
`A duplicate ng-template name "${templateAttr.name}" was found. ` +
`The control flow migration requires unique ng-template names within a component.`);
}
}
super.visitElement(el, null);
Expand Down
60 changes: 32 additions & 28 deletions packages/core/schematics/ng-generate/control-flow-migration/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -309,40 +309,44 @@ function wrapIntoI18nContainer(i18nAttr: Attribute, content: string) {
/**
* Counts, replaces, and removes any necessary ng-templates post control flow migration
*/
export function processNgTemplates(template: string): string {
export function processNgTemplates(template: string): {migrated: string, err: Error|undefined} {
// count usage
const templates = getTemplates(template);

// swap placeholders and remove
for (const [name, t] of templates) {
const replaceRegex = new RegExp(`${name}\\|`, 'g');
const forRegex = new RegExp(`${name}\\#`, 'g');
const forMatches = [...template.matchAll(forRegex)];
const matches = [...forMatches, ...template.matchAll(replaceRegex)];
let safeToRemove = true;
if (matches.length > 0) {
if (t.i18n !== null) {
const container = wrapIntoI18nContainer(t.i18n, t.children);
template = template.replace(replaceRegex, container);
} else if (t.children.trim() === '' && t.isNgTemplateOutlet) {
template = template.replace(replaceRegex, t.generateTemplateOutlet());
} else if (forMatches.length > 0) {
if (t.count === 2) {
template = template.replace(forRegex, t.children);
try {
const templates = getTemplates(template);

// swap placeholders and remove
for (const [name, t] of templates) {
const replaceRegex = new RegExp(`${name}\\|`, 'g');
const forRegex = new RegExp(`${name}\\#`, 'g');
const forMatches = [...template.matchAll(forRegex)];
const matches = [...forMatches, ...template.matchAll(replaceRegex)];
let safeToRemove = true;
if (matches.length > 0) {
if (t.i18n !== null) {
const container = wrapIntoI18nContainer(t.i18n, t.children);
template = template.replace(replaceRegex, container);
} else if (t.children.trim() === '' && t.isNgTemplateOutlet) {
template = template.replace(replaceRegex, t.generateTemplateOutlet());
} else if (forMatches.length > 0) {
if (t.count === 2) {
template = template.replace(forRegex, t.children);
} else {
template = template.replace(forRegex, t.generateTemplateOutlet());
safeToRemove = false;
}
} else {
template = template.replace(forRegex, t.generateTemplateOutlet());
safeToRemove = false;
template = template.replace(replaceRegex, t.children);
}
// 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, '');
}
} else {
template = template.replace(replaceRegex, t.children);
}
// 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, '');
}
}
return {migrated: template, err: undefined};
} catch (err) {
return {migrated: template, err: err as Error};
}
return template;
}

/**
Expand Down
30 changes: 30 additions & 0 deletions packages/core/schematics/test/control_flow_migration_spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3190,6 +3190,36 @@ describe('control flow migration', () => {
expect(warnOutput.join(' '))
.toContain('IMPORTANT! This migration is in developer preview. Use with caution.');
});

it('should log a migration error when duplicate ng-template names are detected', async () => {
writeFile('/comp.ts', `
import {Component} from '@angular/core';
import {NgIf} from '@angular/common';
@Component({
imports: [NgIf],
templateUrl: './comp.html'
})
class Comp {
toggle = false;
}
`);

writeFile('./comp.html', [
`<div *ngIf="show; else elseTmpl">Content</div>`,
`<div *ngIf="hide; else elseTmpl">Content</div>`,
`<ng-template #elseTmpl>Else Content</ng-template>`,
`<ng-template #elseTmpl>Duplicate</ng-template>`,
].join('\n'));

await runMigration();
tree.readContent('/comp.ts');

expect(warnOutput.join(' '))
.toContain(
`A duplicate ng-template name "#elseTmpl" was found. ` +
`The control flow migration requires unique ng-template names within a component.`);
});
});

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

0 comments on commit 0fcef65

Please sign in to comment.