Skip to content

Commit 434c0c9

Browse files
committed
fix(migrations): handle reused templates in control flow migration
The control flow migration was incorrectly removing `ng-template` elements in scenarios where they were referenced by multiple `*ngIf` directives' `else` clauses and also used independently via `ngTemplateOutlet`.
1 parent 5406e1a commit 434c0c9

2 files changed

Lines changed: 125 additions & 40 deletions

File tree

packages/core/schematics/migrations/control-flow-migration/util.ts

Lines changed: 87 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,11 @@ const startI18nMarkerRegex = new RegExp(startI18nMarker, 'gm');
3333
const endI18nMarkerRegex = new RegExp(endI18nMarker, 'gm');
3434
const replaceMarkerRegex = new RegExp(`${startMarker}|${endMarker}`, 'gm');
3535

36+
interface TemplateUsageResult {
37+
isReferencedInTemplateOutlet: boolean;
38+
totalCount: number;
39+
}
40+
3641
/**
3742
* Analyzes a source file to find file that need to be migrated and the text ranges within them.
3843
* @param sourceFile File to be analyzed.
@@ -399,39 +404,6 @@ export function getTemplates(template: string): Map<string, Template> {
399404
return new Map<string, Template>();
400405
}
401406

402-
function countTemplateUsage(nodes: any[], templateName: string): number {
403-
let count = 0;
404-
let isReferencedInTemplateOutlet = false;
405-
406-
for (const node of nodes) {
407-
if (node.attrs) {
408-
for (const attr of node.attrs) {
409-
if (attr.name === '*ngTemplateOutlet' && attr.value === templateName.slice(1)) {
410-
isReferencedInTemplateOutlet = true;
411-
break;
412-
}
413-
414-
if (attr.name.trim() === templateName) {
415-
count++;
416-
}
417-
}
418-
}
419-
420-
if (node.children) {
421-
if (node.name === 'for') {
422-
for (const child of node.children) {
423-
if (child.value?.includes(templateName.slice(1))) {
424-
count++;
425-
}
426-
}
427-
}
428-
count += countTemplateUsage(node.children, templateName);
429-
}
430-
}
431-
432-
return isReferencedInTemplateOutlet ? count + 2 : count;
433-
}
434-
435407
export function updateTemplates(
436408
template: string,
437409
templates: Map<string, Template>,
@@ -468,9 +440,13 @@ export function processNgTemplates(
468440
const templates = getTemplates(template);
469441

470442
// swap placeholders and remove
471-
for (const [name, t] of templates) {
472-
const replaceRegex = new RegExp(getPlaceholder(name.slice(1)), 'g');
473-
const forRegex = new RegExp(getPlaceholder(name.slice(1), PlaceholderKind.Alternate), 'g');
443+
for (const [nameWithHash, t] of templates) {
444+
const name = nameWithHash.slice(1);
445+
const replaceRegex = new RegExp(getPlaceholder(name), 'g');
446+
const forRegex = new RegExp(
447+
getPlaceholder(nameWithHash.slice(1), PlaceholderKind.Alternate),
448+
'g',
449+
);
474450
const forMatches = [...template.matchAll(forRegex)];
475451
const matches = [...forMatches, ...template.matchAll(replaceRegex)];
476452
let safeToRemove = true;
@@ -495,7 +471,15 @@ export function processNgTemplates(
495471
(obj, index, self) => index === self.findIndex((t) => t.input === obj.input),
496472
);
497473

498-
if ((t.count === dist.length || t.count - matches.length === 1) && safeToRemove) {
474+
// Check if template is used by ngTemplateOutlet in addition to control flow
475+
const hasTemplateOutletUsage = checkForTemplateOutletUsage(template, nameWithHash.slice(1));
476+
477+
// Only remove template if it's safe to do so AND not used by ngTemplateOutlet
478+
if (
479+
(t.count === dist.length || t.count - matches.length === 1) &&
480+
safeToRemove &&
481+
!hasTemplateOutletUsage
482+
) {
499483
const refsInComponentFile = getViewChildOrViewChildrenNames(sourceFile);
500484
if (refsInComponentFile?.length > 0) {
501485
const templateRefs = getTemplateReferences(template);
@@ -524,6 +508,69 @@ export function processNgTemplates(
524508
}
525509
}
526510

511+
function analyzeTemplateUsage(nodes: any[], templateName: string): TemplateUsageResult {
512+
let count = 0;
513+
let isReferencedInTemplateOutlet = false;
514+
const templateNameWithHash = `#${templateName}`;
515+
516+
function traverseNodes(nodeList: any[]): void {
517+
for (const node of nodeList) {
518+
if (node.attrs) {
519+
for (const attr of node.attrs) {
520+
if (
521+
(attr.name === '*ngTemplateOutlet' || attr.name === '[ngTemplateOutlet]') &&
522+
attr.value === templateName
523+
) {
524+
isReferencedInTemplateOutlet = true;
525+
}
526+
527+
if (attr.name.trim() === templateNameWithHash) {
528+
count++;
529+
}
530+
}
531+
}
532+
533+
if (node.children) {
534+
if (node.name === 'for') {
535+
for (const child of node.children) {
536+
if (child.value?.includes(templateName)) {
537+
count++;
538+
}
539+
}
540+
}
541+
542+
traverseNodes(node.children);
543+
}
544+
}
545+
}
546+
547+
traverseNodes(nodes);
548+
549+
return {
550+
isReferencedInTemplateOutlet,
551+
totalCount: isReferencedInTemplateOutlet ? count + 2 : count,
552+
};
553+
}
554+
555+
/**
556+
* Checks if a template is used by ngTemplateOutlet directive
557+
*/
558+
function checkForTemplateOutletUsage(template: string, templateName: string): boolean {
559+
const parsed = parseTemplate(template);
560+
if (parsed.tree === undefined) {
561+
return false;
562+
}
563+
564+
const result = analyzeTemplateUsage(parsed.tree.rootNodes, templateName);
565+
return result.isReferencedInTemplateOutlet;
566+
}
567+
568+
function countTemplateUsage(nodes: any[], templateNameWithHash: string): number {
569+
const templateName = templateNameWithHash.slice(1);
570+
const result = analyzeTemplateUsage(nodes, templateName);
571+
return result.totalCount;
572+
}
573+
527574
function getViewChildOrViewChildrenNames(sourceFile: ts.SourceFile): Array<string> {
528575
const names: Array<string> = [];
529576

@@ -554,12 +601,12 @@ function getTemplateReferences(template: string): string[] {
554601
return [];
555602
}
556603

557-
const references: string[] = [];
604+
const templateNameRefWithoutHash: string[] = [];
558605

559606
function visitNodes(nodes: any) {
560607
for (const node of nodes) {
561608
if (node?.name === 'ng-template') {
562-
references.push(...node.attrs?.map((ref: any) => ref?.name?.slice(1)));
609+
templateNameRefWithoutHash.push(...node.attrs?.map((ref: any) => ref?.name?.slice(1)));
563610
}
564611
if (node.children) {
565612
visitNodes(node.children);
@@ -568,7 +615,7 @@ function getTemplateReferences(template: string): string[] {
568615
}
569616

570617
visitNodes(parsed.tree.rootNodes);
571-
return references;
618+
return templateNameRefWithoutHash;
572619
}
573620

574621
function replaceRemainingPlaceholders(template: string): string {

packages/core/schematics/test/control_flow_migration_spec.ts

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5074,6 +5074,44 @@ describe('control flow migration (ng update)', () => {
50745074
'@if (show) {<div>Some greek characters: θδ!</div>}',
50755075
);
50765076
});
5077+
5078+
it('should migrate multiple ngIf directives with same else template and preserve template outlet', async () => {
5079+
writeFile(
5080+
'/comp.ts',
5081+
`
5082+
import {Component} from '@angular/core';
5083+
import {NgIf} from '@angular/common';
5084+
5085+
@Component({
5086+
imports: [NgIf],
5087+
template: \`
5088+
<div *ngIf="1 == 1; else elseTemplate">
5089+
<h1>TEST</h1>
5090+
</div>
5091+
<div *ngIf="1 == 1; else elseTemplate">
5092+
<h1>TEST</h1>
5093+
</div>
5094+
5095+
<ng-container [ngTemplateOutlet]="elseTemplate"></ng-container>
5096+
<ng-template #elseTemplate>
5097+
<h1>Test</h1>
5098+
<div>Test</div>
5099+
</ng-template>
5100+
\`
5101+
})
5102+
class Comp {
5103+
}
5104+
`,
5105+
);
5106+
5107+
await runMigration();
5108+
const content = tree.readContent('/comp.ts');
5109+
5110+
expect(content.replace(/\s+/g, ' ')).toContain(
5111+
`<ng-container [ngTemplateOutlet]="elseTemplate"></ng-container>`,
5112+
);
5113+
expect(content.replace(/\s+/g, ' ')).toContain(`<ng-template #elseTemplate>`);
5114+
});
50775115
});
50785116

50795117
describe('formatting', () => {

0 commit comments

Comments
 (0)