fix(migrations): cf migration removes unnecessary bound ngifelse attribute (#53236)

thePunderWoman · pkozlowski-opensource · commit dffeac8386be · 2023-11-29T09:47:45.000+01:00
this removes a no longer necessary attribute in bound ngIfElse cases. fixes: #53230 PR Close #53236
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/types.ts b/packages/core/schematics/ng-generate/control-flow-migration/types.ts
@@ -141,6 +141,35 @@ export class ElementToMigrate {
         .trim();
   }
 
+  getValueEnd(offset: number): number {
+    return (this.attr.valueSpan ? (this.attr.valueSpan.end.offset + 1) :
+                                  this.attr.keySpan!.end.offset) -
+        offset;
+  }
+
+  hasChildren(): boolean {
+    return this.el.children.length > 0;
+  }
+
+  getChildSpan(offset: number): {childStart: number, childEnd: number} {
+    const childStart = this.el.children[0].sourceSpan.start.offset - offset;
+    const childEnd = this.el.children[this.el.children.length - 1].sourceSpan.end.offset - offset;
+    return {childStart, childEnd};
+  }
+
+  shouldRemoveElseAttr(): boolean {
+    return (this.el.name === 'ng-template' || this.el.name === 'ng-container') &&
+        this.elseAttr !== undefined;
+  }
+
+  getElseAttrStr(): string {
+    if (this.elseAttr !== undefined) {
+      const elseValStr = this.elseAttr.value !== '' ? `="${this.elseAttr.value}"` : '';
+      return `${this.elseAttr.name}${elseValStr}`;
+    }
+    return '';
+  }
+
   start(offset: number): number {
     return this.el.sourceSpan?.start.offset - offset;
   }
diff --git a/packages/core/schematics/ng-generate/control-flow-migration/util.ts b/packages/core/schematics/ng-generate/control-flow-migration/util.ts
@@ -428,35 +428,40 @@ function isRemovableContainer(etm: ElementToMigrate): boolean {
 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');
+
+  // removable containers are ng-templates or ng-containers that no longer need to exist
+  // post migration
   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: ''};
+    const {childStart, childEnd} = etm.getChildSpan(offset);
+    return {start: '', middle: tmpl.slice(childStart, childEnd), end: ''};
   } 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;
+    // here we're removing an ng-template used for control flow and i18n and
+    // converting it to an ng-container with i18n
+    const {childStart, childEnd} = etm.getChildSpan(offset);
     return generatei18nContainer(i18nAttr!, tmpl.slice(childStart, childEnd));
   }
 
+  // the index of the start of the attribute adjusting for offset shift
   const attrStart = etm.attr.keySpan!.start.offset - 1 - offset;
-  const valEnd =
-      (etm.attr.valueSpan ? (etm.attr.valueSpan.end.offset + 1) : etm.attr.keySpan!.end.offset) -
-      offset;
 
-  let childStart = valEnd;
-  let childEnd = valEnd;
+  // the index of the very end of the attribute value adjusted for offset shift
+  const valEnd = etm.getValueEnd(offset);
 
-  if (etm.el.children.length > 0) {
-    childStart = etm.el.children[0].sourceSpan.start.offset - offset;
-    childEnd = etm.el.children[etm.el.children.length - 1].sourceSpan.end.offset - offset;
-  }
+  // the index of the children start and end span, if they exist. Otherwise use the value end.
+  const {childStart, childEnd} =
+      etm.hasChildren() ? etm.getChildSpan(offset) : {childStart: valEnd, childEnd: valEnd};
 
-  let start = tmpl.slice(etm.start(offset), attrStart);
-  start += tmpl.slice(valEnd, childStart);
+  // the beginning of the updated string in the main block, for example: <div some="attributes">
+  let start = tmpl.slice(etm.start(offset), attrStart) + tmpl.slice(valEnd, childStart);
+
+  if (etm.shouldRemoveElseAttr()) {
+    // this removes a bound ngIfElse attribute that's no longer needed
+    start = start.replace(etm.getElseAttrStr(), '');
+  }
 
+  // the middle is the actual contents of the element
   const middle = tmpl.slice(childStart, childEnd);
+  // the end is the closing part of the element, example: </div>
   const end = tmpl.slice(childEnd, etm.end(offset));
 
   return {start, middle, end};
diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts
@@ -3445,6 +3445,53 @@ describe('control flow migration', () => {
       expect(content).toBe(result);
     });
 
+    it('should migrate template with ngTemplateOutlet on if else template', async () => {
+      writeFile('/comp.ts', `
+        import {Component} from '@angular/core';
+        import {NgIf} from '@angular/common';
+
+        @Component({
+          selector: 'declare-comp',
+          templateUrl: 'comp.html',
+        })
+        class DeclareComp {}
+      `);
+
+      writeFile('/comp.html', [
+        `<ng-template`,
+        `  [ngIf]="false"`,
+        `  [ngIfElse]="fooTemplate"`,
+        `  [ngTemplateOutlet]="barTemplate"`,
+        `>`,
+        `</ng-template>`,
+        `<ng-template #fooTemplate>`,
+        `  Foo`,
+        `</ng-template>`,
+        `<ng-template #barTemplate>`,
+        `  Bar`,
+        `</ng-template>`,
+      ].join('\n'));
+
+      await runMigration();
+      const content = tree.readContent('/comp.html');
+
+      const result = [
+        `@if (false) {`,
+        `  <ng-template`,
+        `    [ngTemplateOutlet]="barTemplate"`,
+        `    >`,
+        `  </ng-template>`,
+        `} @else {`,
+        `  Foo`,
+        `}`,
+        `<ng-template #barTemplate>`,
+        `  Bar`,
+        `</ng-template>`,
+      ].join('\n');
+
+      expect(content).toBe(result);
+    });
+
     it('should move templates after they were migrated to new syntax', async () => {
       writeFile('/comp.ts', `
         import {Component} from '@angular/core';
