fix(migrations): Update CF migration to skip templates with duplicate ng-template names (#53204)

thePunderWoman · pkozlowski-opensource · commit 8a52674faace · 2023-11-28T16:43:28.000+01:00
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
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/migration.ts b/packages/core/schematics/ng-generate/control-flow-migration/migration.ts
@@ -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) {
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/types.ts b/packages/core/schematics/ng-generate/control-flow-migration/types.ts
@@ -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);
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/util.ts b/packages/core/schematics/ng-generate/control-flow-migration/util.ts
@@ -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;
 }
 
 /**
diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts
@@ -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', () => {

Original file line number	Diff line number	Diff line change
`@@ -338,9 +338,13 @@ export class TemplateCollector extends RecursiveVisitor {`
`338`	`338`	`templateAttr = attr;`
`339`	`339`	`}`
`340`	`340`	`}`
`341`		`- if (templateAttr !== null) {`
`342`		`- this.elements.push(new ElementToMigrate(el, templateAttr));`
	`341`	`+ if (templateAttr !== null && !this.templates.has(templateAttr.name)) {`
`343`	`342`	`this.templates.set(templateAttr.name, new Template(el, templateAttr.name, i18n));`
	`343`	`+ this.elements.push(new ElementToMigrate(el, templateAttr));`
	`344`	`+ } else if (templateAttr !== null) {`
	`345`	`+ throw new Error(`
	`346`	+ `A duplicate ng-template name "${templateAttr.name}" was found. ` +
	`347`	+ `The control flow migration requires unique ng-template names within a component.`);
`344`	`348`	`}`
`345`	`349`	`}`
`346`	`350`	`super.visitElement(el, null);`