From d0e2d9dd048a82abdb1c1dcc6f81502dbdc4d0d9 Mon Sep 17 00:00:00 2001 From: Jessica Janiuk Date: Tue, 5 Dec 2023 13:02:16 -0500 Subject: [PATCH 1/2] fix(migrations): handle nested ng-template replacement safely in CF migration When there are ng-templates nested inside other ng-templates, the replacement and removal of the templates gets disrupted. Re-processing the templates in the file along the way resolves this issue. fixes: #53362 --- .../control-flow-migration/util.ts | 14 ++++++- .../test/control_flow_migration_spec.ts | 42 +++++++++++++++++++ 2 files changed, 55 insertions(+), 1 deletion(-) diff --git a/packages/core/schematics/ng-generate/control-flow-migration/util.ts b/packages/core/schematics/ng-generate/control-flow-migration/util.ts index 86b7e2b692b8..e0264d4c022e 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/util.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/util.ts @@ -284,7 +284,7 @@ export function getTemplates(template: string): Map { // count usages of each ng-template for (let [key, tmpl] of visitor.templates) { const escapeKey = escapeRegExp(key.slice(1)); - const regex = new RegExp(`[^a-zA-Z0-9-<\']${escapeKey}\\W`, 'gm'); + const regex = new RegExp(`[^a-zA-Z0-9-<(\']${escapeKey}\\W`, 'gm'); const matches = template.match(regex); tmpl.count = matches?.length ?? 0; tmpl.generateContents(template); @@ -295,6 +295,15 @@ export function getTemplates(template: string): Map { return new Map(); } +export function updateTemplates( + template: string, templates: Map): Map { + const updatedTemplates = getTemplates(template); + for (let [key, tmpl] of updatedTemplates) { + templates.set(key, tmpl); + } + return templates; +} + function wrapIntoI18nContainer(i18nAttr: Attribute, content: string) { const {start, middle, end} = generatei18nContainer(i18nAttr, content); return `${start}${middle}${end}`; @@ -341,6 +350,9 @@ export function processNgTemplates(template: string): {migrated: string, err: Er if (t.count === matches.length + 1 && safeToRemove) { template = template.replace(t.contents, ''); } + // templates may have changed structure from nested replaced templates + // so we need to reprocess them before the next loop. + updateTemplates(template, templates); } } return {migrated: template, err: undefined}; diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts index d28fe664dd26..9af197dd0e1a 100644 --- a/packages/core/schematics/test/control_flow_migration_spec.ts +++ b/packages/core/schematics/test/control_flow_migration_spec.ts @@ -3970,6 +3970,48 @@ describe('control flow migration', () => { `}`, ].join('\n')); }); + + it('should migrate nested template usage correctly', 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', [ + ``, + ` Hello!`, + ``, + `Bar`, + `Foo`, + ``, + ` `, + ``, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe([ + `@if (!(condition$ | async)) {`, + ` Hello!`, + `} @else {`, + ` @if ((foo$ | async) === true) {`, + ` Foo`, + ` } @else {`, + ` Bar`, + ` }`, + `}\n`, + ].join('\n')); + }); }); describe('formatting', () => { From 2639410cc507f6d2ea505c67a66cc36c6a684eb0 Mon Sep 17 00:00:00 2001 From: Jessica Janiuk Date: Tue, 5 Dec 2023 13:05:25 -0500 Subject: [PATCH 2/2] fix(migrations): handle templates outside of component in cf migration If a template is passed in as an input, the ng-template will not exist in the same component template. This will leave a template placeholder behind. This fix ensures that template placeholder gets turned into a template outlet. fixes: #53361 --- .../control-flow-migration/util.ts | 18 ++++++++++ .../test/control_flow_migration_spec.ts | 34 +++++++++++++++++++ 2 files changed, 52 insertions(+) diff --git a/packages/core/schematics/ng-generate/control-flow-migration/util.ts b/packages/core/schematics/ng-generate/control-flow-migration/util.ts index e0264d4c022e..87e8ecfec9a7 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/util.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/util.ts @@ -355,12 +355,30 @@ export function processNgTemplates(template: string): {migrated: string, err: Er updateTemplates(template, templates); } } + // template placeholders may still exist if the ng-template name is not + // present in the component. This could be because it's passed in from + // another component. In that case, we need to replace any remaining + // template placeholders with template outlets. + template = replaceRemainingPlaceholders(template); return {migrated: template, err: undefined}; } catch (err) { return {migrated: template, err: err as Error}; } } +function replaceRemainingPlaceholders(template: string): string { + const replaceRegex = new RegExp(`#\\w*\\|`, 'g'); + const placeholders = [...template.matchAll(replaceRegex)]; + let migrated = template; + for (let ph of placeholders) { + const placeholder = ph[0]; + const name = placeholder.slice(1, placeholder.length - 1); + migrated = + template.replace(placeholder, ``); + } + return migrated; +} + /** * determines if the CommonModule can be safely removed from imports */ diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts index 9af197dd0e1a..aac93e7096f6 100644 --- a/packages/core/schematics/test/control_flow_migration_spec.ts +++ b/packages/core/schematics/test/control_flow_migration_spec.ts @@ -4012,6 +4012,40 @@ describe('control flow migration', () => { `}\n`, ].join('\n')); }); + + it('should add an ngTemplateOutlet when the template placeholder does not match a template', + 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', [ + ``, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe([ + `@if (active) {`, + ` `, + `} @else {`, + ` `, + `}`, + ].join('\n')); + }); }); describe('formatting', () => {