fix(migrations): Add missing support for ngForOf (#52903)

This adds support to migrate ngForOf and ngForTrackBy when migrating control flow.

PR Close #52903

packages/core/schematics/ng-generate/control-flow-migration/fors.ts

@@ -12,6 +12,12 @@ import {ElementCollector, ElementToMigrate, MigrateError, Result} from './types'
 import {calculateNesting, getMainBlock, getOriginals, hasLineBreaks, parseTemplate, reduceNestingOffset} from './util';
 
 export const ngfor = '*ngFor';
+export const nakedngfor = 'ngFor';
+const fors = [
+  ngfor,
+  nakedngfor,
+];
+
 export const commaSeparatedSyntax = new Map([
   ['(', ')'],
   ['{', '}'],
@@ -34,7 +40,7 @@ export function migrateFor(template: string): {migrated: string, errors: Migrate
   }
 
   let result = template;
-  const visitor = new ElementCollector([ngfor]);
+  const visitor = new ElementCollector(fors);
   visitAll(visitor, parsed.rootNodes);
   calculateNesting(visitor, hasLineBreaks(template));
 
@@ -65,8 +71,14 @@ export function migrateFor(template: string): {migrated: string, errors: Migrate
   return {migrated: result, errors};
 }
 
-
 function migrateNgFor(etm: ElementToMigrate, tmpl: string, offset: number): Result {
+  if (etm.forAttrs !== undefined) {
+    return migrateBoundNgFor(etm, tmpl, offset);
+  }
+  return migrateStandardNgFor(etm, tmpl, offset);
+}
+
+function migrateStandardNgFor(etm: ElementToMigrate, tmpl: string, offset: number): Result {
   const aliasWithEqualRegexp = /=\s*(count|index|first|last|even|odd)/gm;
   const aliasWithAsRegexp = /(count|index|first|last|even|odd)\s+as/gm;
   const aliases = [];
@@ -136,6 +148,43 @@ function migrateNgFor(etm: ElementToMigrate, tmpl: string, offset: number): Resu
   return {tmpl: updatedTmpl, offsets: {pre, post}};
 }
 
+function migrateBoundNgFor(etm: ElementToMigrate, tmpl: string, offset: number): Result {
+  const forAttrs = etm.forAttrs!;
+  const aliasMap = forAttrs.aliases;
+
+  const originals = getOriginals(etm, tmpl, offset);
+  const condition = `${forAttrs.item} of ${forAttrs.forOf}`;
+
+  const aliases = [];
+  let aliasedIndex = '$index';
+  for (const [key, val] of aliasMap) {
+    aliases.push(` let ${key.trim()} = $${val}`);
+    if (val.trim() === 'index') {
+      aliasedIndex = key;
+    }
+  }
+  const aliasStr = (aliases.length > 0) ? `;${aliases.join(';')}` : '';
+
+  let trackBy = forAttrs.item;
+  if (forAttrs.trackBy !== '') {
+    // build trackby value
+    trackBy = `${forAttrs.trackBy.trim()}(${aliasedIndex}, ${forAttrs.item})`;
+  }
+
+  const {start, middle, end} = getMainBlock(etm, tmpl, offset);
+  const startBlock = `@for (${condition}; track ${trackBy}${aliasStr}) {\n  ${start}`;
+
+  const endBlock = `${end}\n}`;
+  const forBlock = startBlock + middle + endBlock;
+
+  const updatedTmpl = tmpl.slice(0, etm.start(offset)) + forBlock + tmpl.slice(etm.end(offset));
+
+  const pre = originals.start.length - startBlock.length;
+  const post = originals.end.length - endBlock.length;
+
+  return {tmpl: updatedTmpl, offsets: {pre, post}};
+}
+
 function getNgForParts(expression: string): string[] {
   const parts: string[] = [];
   const commaSeparatedStack: string[] = [];

packages/core/schematics/ng-generate/control-flow-migration/ifs.ts

@@ -15,7 +15,7 @@ export const ngif = '*ngIf';
 export const boundngif = '[ngIf]';
 export const nakedngif = 'ngIf';
 
-export const ifs = [
+const ifs = [
   ngif,
   nakedngif,
   boundngif,

packages/core/schematics/ng-generate/control-flow-migration/switches.ts

@@ -18,7 +18,7 @@ export const nakedcase = 'ngSwitchCase';
 export const switchdefault = '*ngSwitchDefault';
 export const nakeddefault = 'ngSwitchDefault';
 
-export const switches = [
+const switches = [
   ngswitch,
   boundcase,
   switchcase,

packages/core/schematics/ng-generate/control-flow-migration/types.ts

@@ -12,6 +12,7 @@ import ts from 'typescript';
 export const ngtemplate = 'ng-template';
 export const boundngifelse = '[ngIfElse]';
 export const boundngifthenelse = '[ngIfThenElse]';
+export const nakedngfor = 'ngFor';
 
 function allFormsOf(selector: string): string[] {
   return [
@@ -30,6 +31,10 @@ const commonModuleDirectives = new Set([
   ...allFormsOf('ngStyle'),
   ...allFormsOf('ngTemplateOutlet'),
   ...allFormsOf('ngComponentOutlet'),
+  '[NgForOf]',
+  '[NgForTrackBy]',
+  '[ngIfElse]',
+  '[ngIfThenElse]',
 ]);
 
 function pipeMatchRegExpFor(name: string): RegExp {
@@ -72,6 +77,13 @@ export type Result = {
   offsets: Offsets,
 };
 
+export interface ForAttributes {
+  forOf: string;
+  trackBy: string;
+  item: string;
+  aliases: Map<string, string>;
+}
+
 /**
  * Represents an error that happened during migration
  */
@@ -88,16 +100,18 @@ export class ElementToMigrate {
   attr: Attribute;
   elseAttr: Attribute|undefined;
   thenAttr: Attribute|undefined;
+  forAttrs: ForAttributes|undefined;
   nestCount = 0;
   hasLineBreaks = false;
 
   constructor(
       el: Element, attr: Attribute, elseAttr: Attribute|undefined = undefined,
-      thenAttr: Attribute|undefined = undefined) {
+      thenAttr: Attribute|undefined = undefined, forAttrs: ForAttributes|undefined = undefined) {
     this.el = el;
     this.attr = attr;
     this.elseAttr = elseAttr;
     this.thenAttr = thenAttr;
+    this.forAttrs = forAttrs;
   }
 
   getCondition(targetStr: string): string {
@@ -236,12 +250,38 @@ export class ElementCollector extends RecursiveVisitor {
         if (this._attributes.includes(attr.name)) {
           const elseAttr = el.attrs.find(x => x.name === boundngifelse);
           const thenAttr = el.attrs.find(x => x.name === boundngifthenelse);
-          this.elements.push(new ElementToMigrate(el, attr, elseAttr, thenAttr));
+          const forAttrs = attr.name === nakedngfor ? this.getForAttrs(el) : undefined;
+          this.elements.push(new ElementToMigrate(el, attr, elseAttr, thenAttr, forAttrs));
         }
       }
     }
     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) {
+      if (attr.name === '[ngForTrackBy]') {
+        trackBy = attr.value;
+      }
+      if (attr.name === '[ngForOf]') {
+        forOf = attr.value;
+      }
+      if (attr.name.startsWith('let-')) {
+        if (attr.value === '') {
+          // item
+          item = attr.name.replace('let-', '');
+        } else {
+          // alias
+          aliases.set(attr.name.replace('let-', ''), attr.value);
+        }
+      }
+    }
+    return {forOf, trackBy, item, aliases};
+  }
 }
 
 /** Finds all elements with ngif structural directives. */

packages/core/schematics/ng-generate/control-flow-migration/util.ts

@@ -12,7 +12,10 @@ import ts from 'typescript';
 
 import {AnalyzedFile, CommonCollector, ElementCollector, ElementToMigrate, Template, TemplateCollector} from './types';
 
-const importRemovals = ['NgIf', 'NgFor', 'NgSwitch', 'NgSwitchCase', 'NgSwitchDefault'];
+const importRemovals = [
+  'NgIf', 'NgIfElse', 'NgIfThenElse', 'NgFor', 'NgForOf', 'NgForTrackBy', 'NgSwitch',
+  'NgSwitchCase', 'NgSwitchDefault'
+];
 const importWithCommonRemovals = [...importRemovals, 'CommonModule'];
 
 /**
@@ -353,26 +356,34 @@ export function getOriginals(
   return {start, end: ''};
 }
 
+function isI18nTemplate(etm: ElementToMigrate, i18nAttr: Attribute|undefined): boolean {
+  return etm.el.name === 'ng-template' && i18nAttr !== undefined &&
+      (etm.el.attrs.length === 2 || (etm.el.attrs.length === 3 && etm.elseAttr !== undefined));
+}
+
+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));
+}
+
 /**
  * builds the proper contents of what goes inside a given control flow block after migration
  */
 export function getMainBlock(etm: ElementToMigrate, tmpl: string, offset: number):
     {start: string, middle: string, end: string} {
   const i18nAttr = etm.el.attrs.find(x => x.name === 'i18n');
-  if ((etm.el.name === 'ng-container' || etm.el.name === 'ng-template') &&
-      (etm.el.attrs.length === 1 || (etm.el.attrs.length === 2 && etm.elseAttr !== undefined) ||
-       (etm.el.attrs.length === 3 && etm.elseAttr !== undefined && etm.thenAttr !== undefined))) {
+  if (isRemovableContainer(etm)) {
     // this is the case where we're migrating and there's no need to keep the ng-container
     const childStart = etm.el.children[0].sourceSpan.start.offset - offset;
     const childEnd = etm.el.children[etm.el.children.length - 1].sourceSpan.end.offset - offset;
     const middle = tmpl.slice(childStart, childEnd);
     return {start: '', middle, end: ''};
-  } else if (
-      etm.el.name === 'ng-template' && i18nAttr !== undefined &&
-      (etm.el.attrs.length === 2 || (etm.el.attrs.length === 3 && etm.elseAttr !== undefined))) {
+  } else if (isI18nTemplate(etm, i18nAttr)) {
     const childStart = etm.el.children[0].sourceSpan.start.offset - offset;
     const childEnd = etm.el.children[etm.el.children.length - 1].sourceSpan.end.offset - offset;
-    const middle = wrapIntoI18nContainer(i18nAttr, tmpl.slice(childStart, childEnd));
+    const middle = wrapIntoI18nContainer(i18nAttr!, tmpl.slice(childStart, childEnd));
     return {start: '', middle, end: ''};
   }
 

packages/core/schematics/test/control_flow_migration_spec.ts

@@ -1585,6 +1585,208 @@ describe('control flow migration', () => {
     });
   });
 
+  describe('ngForOf', () => {
+    it('should migrate a basic ngForOf', async () => {
+      writeFile('/comp.ts', `
+        import {Component} from '@angular/core';
+        import {NgFor} from '@angular/common';
+        interface Item {
+          id: number;
+          text: string;
+        }
+
+        @Component({
+          imports: [NgFor,NgForOf],
+          templateUrl: 'comp.html',
+        })
+        class Comp {
+          items: Item[] = [{id: 1, text: 'blah'},{id: 2, text: 'stuff'}];
+        }
+      `);
+
+      writeFile('/comp.html', [
+        `<tbody>`,
+        `  <ng-template ngFor let-rowData [ngForOf]="things">`,
+        `    <tr><td>{{rowData}}</td></tr>`,
+        `  </ng-template>`,
+        `</tbody>`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.html');
+
+      const expected = [
+        `<tbody>`,
+        `  @for (rowData of things; track rowData) {\n  `,
+        `    <tr><td>{{rowData}}</td></tr>\n  `,
+        `}`,
+        `</tbody>`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
+
+    it('should migrate ngForOf with an alias', async () => {
+      writeFile('/comp.ts', `
+        import {Component} from '@angular/core';
+        import {NgFor} from '@angular/common';
+        interface Item {
+          id: number;
+          text: string;
+        }
+
+        @Component({
+          imports: [NgFor,NgForOf],
+          templateUrl: 'comp.html',
+        })
+        class Comp {
+          items: Item[] = [{id: 1, text: 'blah'},{id: 2, text: 'stuff'}];
+        }
+      `);
+
+      writeFile('/comp.html', [
+        `<tbody>`,
+        `  <ng-template ngFor let-rowData let-rowIndex="index" [ngForOf]="things">`,
+        `    <tr><td>{{rowIndex}}</td><td>{{rowData}}</td></tr>`,
+        `  </ng-template>`,
+        `</tbody>`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.html');
+
+      const expected = [
+        `<tbody>`,
+        `  @for (rowData of things; track rowData; let rowIndex = $index) {\n  `,
+        `    <tr><td>{{rowIndex}}</td><td>{{rowData}}</td></tr>\n  `,
+        `}`,
+        `</tbody>`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
+
+    it('should migrate ngForOf with track by', async () => {
+      writeFile('/comp.ts', `
+        import {Component} from '@angular/core';
+        import {NgFor} from '@angular/common';
+        interface Item {
+          id: number;
+          text: string;
+        }
+
+        @Component({
+          imports: [NgFor,NgForOf],
+          templateUrl: 'comp.html',
+        })
+        class Comp {
+          items: Item[] = [{id: 1, text: 'blah'},{id: 2, text: 'stuff'}];
+        }
+      `);
+
+      writeFile('/comp.html', [
+        `<tbody>`,
+        `  <ng-template ngFor let-rowData [ngForOf]="things" [ngForTrackBy]="trackMe">`,
+        `    <tr><td>{{rowData}}</td></tr>`,
+        `  </ng-template>`,
+        `</tbody>`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.html');
+
+      const expected = [
+        `<tbody>`,
+        `  @for (rowData of things; track trackMe($index, rowData)) {\n  `,
+        `    <tr><td>{{rowData}}</td></tr>\n  `,
+        `}`,
+        `</tbody>`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
+
+    it('should migrate ngForOf with track by and alias', async () => {
+      writeFile('/comp.ts', `
+        import {Component} from '@angular/core';
+        import {NgFor} from '@angular/common';
+        interface Item {
+          id: number;
+          text: string;
+        }
+
+        @Component({
+          imports: [NgFor,NgForOf],
+          templateUrl: 'comp.html',
+        })
+        class Comp {
+          items: Item[] = [{id: 1, text: 'blah'},{id: 2, text: 'stuff'}];
+        }
+      `);
+
+      writeFile('/comp.html', [
+        `<tbody>`,
+        `  <ng-template ngFor let-rowData let-rowIndex="index" [ngForOf]="things" [ngForTrackBy]="trackMe">`,
+        `    <tr><td>{{rowIndex}}</td><td>{{rowData}}</td></tr>`,
+        `  </ng-template>`,
+        `</tbody>`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.html');
+
+      const expected = [
+        `<tbody>`,
+        `  @for (rowData of things; track trackMe(rowIndex, rowData); let rowIndex = $index) {\n  `,
+        `    <tr><td>{{rowIndex}}</td><td>{{rowData}}</td></tr>\n  `,
+        `}`,
+        `</tbody>`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
+
+    it('should migrate ngForOf with track by and multiple aliases', async () => {
+      writeFile('/comp.ts', `
+        import {Component} from '@angular/core';
+        import {NgFor} from '@angular/common';
+        interface Item {
+          id: number;
+          text: string;
+        }
+
+        @Component({
+          imports: [NgFor,NgForOf],
+          templateUrl: 'comp.html',
+        })
+        class Comp {
+          items: Item[] = [{id: 1, text: 'blah'},{id: 2, text: 'stuff'}];
+        }
+      `);
+
+      writeFile('/comp.html', [
+        `<tbody>`,
+        `  <ng-template ngFor let-rowData let-rowIndex="index" let-rCount="count" [ngForOf]="things" [ngForTrackBy]="trackMe">`,
+        `    <tr><td>{{rowIndex}}</td><td>{{rowData}}</td></tr>`,
+        `  </ng-template>`,
+        `</tbody>`,
+      ].join('\n'));
+
+      await runMigration();
+      const actual = tree.readContent('/comp.html');
+
+      const expected = [
+        `<tbody>`,
+        `  @for (rowData of things; track trackMe(rowIndex, rowData); let rowIndex = $index; let rCount = $count) {\n  `,
+        `    <tr><td>{{rowIndex}}</td><td>{{rowData}}</td></tr>\n  `,
+        `}`,
+        `</tbody>`,
+      ].join('\n');
+
+      expect(actual).toBe(expected);
+    });
+  });
+
   describe('ngSwitch', () => {
     it('should migrate an inline template', async () => {
       writeFile(