fix(migrations): handle aliases on bound ngIf migrations (#53261)

thePunderWoman · dylhunn · commit f4a96a916092 · 2023-11-30T09:19:15.000-08:00
This fixes a reported issue where ngIf is used on an ng-template with let aliases. fixes: #53251 PR Close #53261
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/fors.ts b/packages/core/schematics/ng-generate/control-flow-migration/fors.ts
@@ -173,10 +173,11 @@ function migrateStandardNgFor(etm: ElementToMigrate, tmpl: string, offset: numbe
 
 function migrateBoundNgFor(etm: ElementToMigrate, tmpl: string, offset: number): Result {
   const forAttrs = etm.forAttrs!;
-  const aliasMap = forAttrs.aliases;
+  const aliasAttrs = etm.aliasAttrs!;
+  const aliasMap = aliasAttrs.aliases;
 
   const originals = getOriginals(etm, tmpl, offset);
-  const condition = `${forAttrs.item} of ${forAttrs.forOf}`;
+  const condition = `${aliasAttrs.item} of ${forAttrs.forOf}`;
 
   const aliases = [];
   let aliasedIndex = '$index';
@@ -188,10 +189,10 @@ function migrateBoundNgFor(etm: ElementToMigrate, tmpl: string, offset: number):
   }
   const aliasStr = (aliases.length > 0) ? `;${aliases.join(';')}` : '';
 
-  let trackBy = forAttrs.item;
+  let trackBy = aliasAttrs.item;
   if (forAttrs.trackBy !== '') {
     // build trackby value
-    trackBy = `${forAttrs.trackBy.trim()}(${aliasedIndex}, ${forAttrs.item})`;
+    trackBy = `${forAttrs.trackBy.trim()}(${aliasedIndex}, ${aliasAttrs.item})`;
   }
 
   const {start, middle, end} = getMainBlock(etm, tmpl, offset);
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/ifs.ts b/packages/core/schematics/ng-generate/control-flow-migration/ifs.ts
@@ -85,12 +85,22 @@ function migrateNgIf(etm: ElementToMigrate, tmpl: string, offset: number): Resul
 }
 
 function buildIfBlock(etm: ElementToMigrate, tmpl: string, offset: number): Result {
+  const aliasAttrs = etm.aliasAttrs!;
+  const aliases = [...aliasAttrs.aliases.keys()];
+
   // includes the mandatory semicolon before as
   const lbString = etm.hasLineBreaks ? '\n' : '';
-  const condition = etm.attr.value
-                        .replace(' as ', '; as ')
-                        // replace 'let' with 'as' whatever spaces are between ; and 'let'
-                        .replace(/;\s*let/g, '; as');
+  let condition = etm.attr.value
+                      .replace(' as ', '; as ')
+                      // replace 'let' with 'as' whatever spaces are between ; and 'let'
+                      .replace(/;\s*let/g, '; as');
+  if (aliases.length > 1 || (aliases.length === 1 && condition.indexOf('; as') > -1)) {
+    // only 1 alias allowed
+    throw new Error(
+        'Found more than one alias on your ngIf. Remove one of them and re-run the migration.');
+  } else if (aliases.length === 1) {
+    condition += `; as ${aliases[0]}`;
+  }
 
   const originals = getOriginals(etm, tmpl, offset);
 
@@ -121,8 +131,18 @@ function buildStandardIfElseBlock(
 }
 
 function buildBoundIfElseBlock(etm: ElementToMigrate, tmpl: string, offset: number): Result {
+  const aliasAttrs = etm.aliasAttrs!;
+  const aliases = [...aliasAttrs.aliases.keys()];
+
   // includes the mandatory semicolon before as
-  const condition = etm.attr.value.replace(' as ', '; as ');
+  let condition = etm.attr.value.replace(' as ', '; as ');
+  if (aliases.length > 1 || (aliases.length === 1 && condition.indexOf('; as') > -1)) {
+    // only 1 alias allowed
+    throw new Error(
+        'Found more than one alias on your ngIf. Remove one of them and re-run the migration.');
+  } else if (aliases.length === 1) {
+    condition += `; as ${aliases[0]}`;
+  }
   const elsePlaceholder = `#${etm.elseAttr!.value}|`;
   if (etm.thenAttr !== undefined) {
     const thenPlaceholder = `#${etm.thenAttr!.value}|`;
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/types.ts b/packages/core/schematics/ng-generate/control-flow-migration/types.ts
@@ -81,6 +81,9 @@ export type Result = {
 export interface ForAttributes {
   forOf: string;
   trackBy: string;
+}
+
+export interface AliasAttributes {
   item: string;
   aliases: Map<string, string>;
 }
@@ -102,17 +105,20 @@ export class ElementToMigrate {
   elseAttr: Attribute|undefined;
   thenAttr: Attribute|undefined;
   forAttrs: ForAttributes|undefined;
+  aliasAttrs: AliasAttributes|undefined;
   nestCount = 0;
   hasLineBreaks = false;
 
   constructor(
       el: Element, attr: Attribute, elseAttr: Attribute|undefined = undefined,
-      thenAttr: Attribute|undefined = undefined, forAttrs: ForAttributes|undefined = undefined) {
+      thenAttr: Attribute|undefined = undefined, forAttrs: ForAttributes|undefined = undefined,
+      aliasAttrs: AliasAttributes|undefined = undefined) {
     this.el = el;
     this.attr = attr;
     this.elseAttr = elseAttr;
     this.thenAttr = thenAttr;
     this.forAttrs = forAttrs;
+    this.aliasAttrs = aliasAttrs;
   }
 
   getCondition(): string {
@@ -319,16 +325,16 @@ export class ElementCollector extends RecursiveVisitor {
           const thenAttr =
               el.attrs.find(x => x.name === boundngifthenelse || x.name === boundngifthen);
           const forAttrs = attr.name === nakedngfor ? this.getForAttrs(el) : undefined;
-          this.elements.push(new ElementToMigrate(el, attr, elseAttr, thenAttr, forAttrs));
+          const aliasAttrs = this.getAliasAttrs(el);
+          this.elements.push(
+              new ElementToMigrate(el, attr, elseAttr, thenAttr, forAttrs, aliasAttrs));
         }
       }
     }
     super.visitElement(el, null);
   }
 
   private getForAttrs(el: Element): ForAttributes {
-    const aliases = new Map<string, string>();
-    let item = '';
     let trackBy = '';
     let forOf = '';
     for (const attr of el.attrs) {
@@ -338,6 +344,14 @@ export class ElementCollector extends RecursiveVisitor {
       if (attr.name === '[ngForOf]') {
         forOf = attr.value;
       }
+    }
+    return {forOf, trackBy};
+  }
+
+  private getAliasAttrs(el: Element): AliasAttributes {
+    const aliases = new Map<string, string>();
+    let item = '';
+    for (const attr of el.attrs) {
       if (attr.name.startsWith('let-')) {
         if (attr.value === '') {
           // item
@@ -348,7 +362,7 @@ export class ElementCollector extends RecursiveVisitor {
         }
       }
     }
-    return {forOf, trackBy, item, aliases};
+    return {item, aliases};
   }
 }
 
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/util.ts b/packages/core/schematics/ng-generate/control-flow-migration/util.ts
@@ -416,10 +416,24 @@ function isI18nTemplate(etm: ElementToMigrate, i18nAttr: Attribute|undefined): b
 }
 
 function isRemovableContainer(etm: ElementToMigrate): boolean {
-  return (etm.el.name === 'ng-container' || etm.el.name === 'ng-template') &&
-      (etm.el.attrs.length === 1 || etm.forAttrs !== undefined ||
-       (etm.el.attrs.length === 2 && etm.elseAttr !== undefined) ||
-       (etm.el.attrs.length === 3 && etm.elseAttr !== undefined && etm.thenAttr !== undefined));
+  let attrCount = countAttributes(etm);
+  const safeToRemove = etm.el.attrs.length === attrCount;
+  return (etm.el.name === 'ng-container' || etm.el.name === 'ng-template') && safeToRemove;
+}
+
+function countAttributes(etm: ElementToMigrate): number {
+  let attrCount = 1;
+  if (etm.elseAttr !== undefined) {
+    attrCount++;
+  }
+  if (etm.thenAttr !== undefined) {
+    attrCount++;
+  }
+  attrCount += etm.aliasAttrs?.aliases.size ?? 0;
+  attrCount += etm.aliasAttrs?.item ? 1 : 0;
+  attrCount += etm.forAttrs?.trackBy ? 1 : 0;
+  attrCount += etm.forAttrs?.forOf ? 1 : 0;
+  return attrCount;
 }
 
 /**
diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts
@@ -327,6 +327,35 @@ describe('control flow migration', () => {
       ].join('\n'));
     });
 
+    it('should migrate an if case as a binding with let variables', async () => {
+      writeFile('/comp.ts', `
+        import {Component} from '@angular/core';
+        import {NgIf} from '@angular/common';
+
+        @Component({
+          templateUrl: './comp.html'
+        })
+        class Comp {
+          show = false;
+        }
+      `);
+
+      writeFile('/comp.html', [
+        `<ng-template [ngIf]="data$ | async" let-data="ngIf">`,
+        `  {{ data }}`,
+        `</ng-template>`,
+      ].join('\n'));
+
+      await runMigration();
+      const content = tree.readContent('/comp.html');
+
+      expect(content).toBe([
+        `@if (data$ | async; as data) {`,
+        `  {{ data }}`,
+        `}`,
+      ].join('\n'));
+    });
+
     it('should migrate an if else case as bindings', async () => {
       writeFile('/comp.ts', `
         import {Component} from '@angular/core';
