From 6a3a7b0d133e36bf191f37f9522bfbfe43c705ea Mon Sep 17 00:00:00 2001 From: Jessica Janiuk Date: Mon, 23 Oct 2023 09:52:00 -0400 Subject: [PATCH] refactor(migrations): Fixes offset calculations for nesting (#52332) This updates offset to handle pre and post offset properly for nested situations, rather than relying on solely nestCount. This should properly apply offset calculations at the right time to handle any nested situation. PR Close #52332 --- .../control-flow-migration/types.ts | 39 +- .../control-flow-migration/util.ts | 385 +++++--- .../test/control_flow_migration_spec.ts | 874 ++++++++++++++++-- 3 files changed, 1044 insertions(+), 254 deletions(-) diff --git a/packages/core/schematics/ng-generate/control-flow-migration/types.ts b/packages/core/schematics/ng-generate/control-flow-migration/types.ts index 21f9a27bbfc..d6904e71db8 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/types.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/types.ts @@ -9,18 +9,25 @@ import {Attribute, Element, RecursiveVisitor} from '@angular/compiler'; export const ngif = '*ngIf'; +export const boundngif = '[ngIf]'; +export const nakedngif = 'ngIf'; export const ngfor = '*ngFor'; export const ngswitch = '[ngSwitch]'; const attributesToMigrate = [ ngif, + nakedngif, + boundngif, ngfor, ngswitch, ]; const casesToMigrate = [ + '[ngSwitchCase]', '*ngSwitchCase', + 'ngSwitchCase', '*ngSwitchDefault', + 'ngSwitchDefault', ]; /** @@ -29,6 +36,16 @@ const casesToMigrate = [ */ type Range = [start: number, end?: number]; +export type Offsets = { + pre: number, + post: number, +}; + +export type Result = { + tmpl: string, + offsets: Offsets, +}; + /** * Represents an error that happened during migration */ @@ -44,6 +61,7 @@ export class ElementToMigrate { el: Element; attr: Attribute; nestCount = 0; + lineBreaks = false; constructor(el: Element, attr: Attribute) { this.el = el; @@ -65,32 +83,16 @@ export class ElementToMigrate { } start(offset: number): number { - return this.el.sourceSpan?.start.offset - this.nestCount - offset; + return this.el.sourceSpan?.start.offset - offset; } end(offset: number): number { - return this.el.sourceSpan?.end.offset - this.nestCount - offset; + return this.el.sourceSpan?.end.offset - offset; } length(): number { return this.el.sourceSpan?.end.offset - this.el.sourceSpan?.start.offset; } - - openLength(): number { - return this.el.children[0]?.sourceSpan.start.offset - this.el.sourceSpan?.start.offset; - } - - closeLength(): number { - return this.el.sourceSpan?.end.offset - this.el.children[0]?.sourceSpan.end.offset; - } - - preOffset(newOffset: number): number { - return newOffset - this.openLength() + 1; - } - - postOffset(newOffset: number): number { - return newOffset - this.closeLength(); - } } export class Template { @@ -98,6 +100,7 @@ export class Template { count: number = 0; contents: string = ''; children: string = ''; + used: boolean = false; constructor(el: Element) { this.el = el; 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 9353ae74e15..5a007d41257 100644 --- a/packages/core/schematics/ng-generate/control-flow-migration/util.ts +++ b/packages/core/schematics/ng-generate/control-flow-migration/util.ts @@ -10,7 +10,7 @@ import {HtmlParser, Node, ParseTreeResult, visitAll} from '@angular/compiler'; import {dirname, join} from 'path'; import ts from 'typescript'; -import {AnalyzedFile, CaseCollector, ElementCollector, ElementToMigrate, MigrateError, ngfor, ngif, ngswitch, Template} from './types'; +import {AnalyzedFile, boundngif, CaseCollector, ElementCollector, ElementToMigrate, MigrateError, nakedngif, ngfor, ngif, ngswitch, Result, Template} from './types'; /** * Analyzes a source file to find file that need to be migrated and the text ranges within them. @@ -86,6 +86,8 @@ function getNestedCount(etm: ElementToMigrate, aggregator: number[]) { } } +const lb = '\n'; + /** * Replaces structural directive control flow instances with block control flow equivalents. * Returns null if the migration failed (e.g. there was a syntax error). @@ -104,6 +106,7 @@ export function migrateTemplate(template: string): {migrated: string|null, error tokenizeExpansionForms: true, // Explicitly disable blocks so that their characters are treated as plain text. tokenizeBlocks: false, + preserveLineEndings: true, }); // Don't migrate invalid templates. @@ -119,6 +122,9 @@ export function migrateTemplate(template: string): {migrated: string|null, error } let result = template; + const lineBreaks = template.match(/\r|\n/g); + const hasLineBreaks = lineBreaks !== null; + const visitor = new ElementCollector(); visitAll(visitor, parsed.rootNodes); @@ -137,6 +143,9 @@ export function migrateTemplate(template: string): {migrated: string|null, error for (let i = 1; i < visitor.elements.length; i++) { let currEl = visitor.elements[i]; currEl.nestCount = getNestedCount(currEl, nestedQueue); + if (currEl.el.sourceSpan.end.offset !== nestedQueue[nestedQueue.length - 1]) { + nestedQueue.push(currEl.el.sourceSpan.end.offset); + } } // this tracks the character shift from different lengths of blocks from @@ -144,38 +153,48 @@ export function migrateTemplate(template: string): {migrated: string|null, error // migration. Each block calculates length differences and passes that offset // to the next migrating block to adjust character offsets properly. let offset = 0; + let nestLevel = -1; + let postOffsets: number[] = []; + let migrateResult: Result = {tmpl: result, offsets: {pre: 0, post: 0}}; for (const el of visitor.elements) { - // these are all migratable nodes + // applies the post offsets after closing + if (el.nestCount <= nestLevel) { + const count = nestLevel - el.nestCount; + // reduced nesting, add postoffset + for (let i = 0; i <= count; i++) { + offset += postOffsets.pop() ?? 0; + } + } - if (el.attr.name === ngif) { + // these are all migratable nodes + if (el.attr.name === ngif || el.attr.name === nakedngif || el.attr.name === boundngif) { try { - let ifResult = migrateNgIf(el, visitor.templates, result, offset); - result = ifResult.tmpl; - offset = ifResult.offset; + migrateResult = migrateNgIf(el, visitor.templates, result, offset, hasLineBreaks); } catch (error: unknown) { - errors.push({type: ngfor, error}); + errors.push({type: ngif, error}); } } else if (el.attr.name === ngfor) { try { - let forResult = migrateNgFor(el, result, offset); - result = forResult.tmpl; - offset = forResult.offset; + migrateResult = migrateNgFor(el, result, offset, hasLineBreaks); } catch (error: unknown) { errors.push({type: ngfor, error}); } } else if (el.attr.name === ngswitch) { try { - let switchResult = migrateNgSwitch(el, result, offset); - result = switchResult.tmpl; - offset = switchResult.offset; + migrateResult = migrateNgSwitch(el, result, offset, hasLineBreaks); } catch (error: unknown) { - errors.push({type: ngfor, error}); + errors.push({type: ngswitch, error}); } } + result = migrateResult.tmpl; + offset += migrateResult.offsets.pre; + postOffsets.push(migrateResult.offsets.post); + const nm = el.el.name; + nestLevel = el.nestCount; } for (const [_, t] of visitor.templates) { - if (t.count < 2) { + if (t.count < 2 && t.used) { result = result.replace(t.contents, ''); } } @@ -183,14 +202,122 @@ export function migrateTemplate(template: string): {migrated: string|null, error return {migrated: result, errors}; } +function migrateNgIf( + etm: ElementToMigrate, ngTemplates: Map, tmpl: string, offset: number, + hasLineBreaks: boolean): Result { + const matchThen = etm.attr.value.match(/;\s+then/gm); + const matchElse = etm.attr.value.match(/;\s+else/gm); + + if (matchThen && matchThen.length > 0) { + return buildIfThenElseBlock( + etm, ngTemplates, tmpl, matchThen[0], matchElse![0], offset, hasLineBreaks); + } else if (matchElse && matchElse.length > 0) { + // just else + return buildIfElseBlock(etm, ngTemplates, tmpl, matchElse[0], offset, hasLineBreaks); + } + + return buildIfBlock(etm, tmpl, offset, hasLineBreaks); +} + +function buildIfBlock( + etm: ElementToMigrate, tmpl: string, offset: number, hasLineBreaks: boolean): Result { + // includes the mandatory semicolon before as + const lbString = hasLineBreaks ? lb : ''; + const condition = etm.attr.value.replace(' as ', '; as '); + + const originals = getOriginals(etm, tmpl, offset); + + const {start, middle, end} = getMainBlock(etm, tmpl, offset); + const startBlock = `@if (${condition}) {${lbString}${start}`; + const endBlock = `${end}${lbString}}`; + + const ifBlock = startBlock + middle + endBlock; + const updatedTmpl = tmpl.slice(0, etm.start(offset)) + ifBlock + tmpl.slice(etm.end(offset)); + + // this should be the difference between the starting element up to the start of the closing + // element and the mainblock sans } + const pre = originals.start.length - startBlock.length; + const post = originals.end.length - endBlock.length; + + return {tmpl: updatedTmpl, offsets: {pre, post}}; +} + +function buildIfElseBlock( + etm: ElementToMigrate, ngTemplates: Map, tmpl: string, elseString: string, + offset: number, hasLineBreaks: boolean): Result { + // includes the mandatory semicolon before as + const lbString = hasLineBreaks ? lb : ''; + const condition = etm.getCondition(elseString).replace(' as ', '; as '); + + const originals = getOriginals(etm, tmpl, offset); + + const elseTmpl = ngTemplates.get(`#${etm.getTemplateName(elseString)}`)!; + const {start, middle, end} = getMainBlock(etm, tmpl, offset); + const startBlock = `@if (${condition}) {${lbString}${start}`; + + const elseBlock = `${end}${lbString}} @else {${lbString}`; + const postBlock = elseBlock + elseTmpl.children + `${lbString}}`; + const ifElseBlock = startBlock + middle + postBlock; + + const tmplStart = tmpl.slice(0, etm.start(offset)); + const tmplEnd = tmpl.slice(etm.end(offset)); + const updatedTmpl = tmplStart + ifElseBlock + tmplEnd; + + // decrease usage count of elseTmpl + elseTmpl.count--; + elseTmpl.used = true; + + const pre = originals.start.length - startBlock.length; + const post = originals.end.length - postBlock.length; + + return {tmpl: updatedTmpl, offsets: {pre, post}}; +} + +function buildIfThenElseBlock( + etm: ElementToMigrate, ngTemplates: Map, tmpl: string, thenString: string, + elseString: string, offset: number, hasLineBreaks: boolean): Result { + const condition = etm.getCondition(thenString).replace(' as ', '; as '); + const lbString = hasLineBreaks ? lb : ''; + + const originals = getOriginals(etm, tmpl, offset); + + const startBlock = `@if (${condition}) {${lbString}`; + const elseBlock = `${lbString}} @else {${lbString}`; + + const thenTmpl = ngTemplates.get(`#${etm.getTemplateName(thenString, elseString)}`)!; + const elseTmpl = ngTemplates.get(`#${etm.getTemplateName(elseString)}`)!; + + const postBlock = thenTmpl.children + elseBlock + elseTmpl.children + `${lbString}}`; + const ifThenElseBlock = startBlock + postBlock; + + const tmplStart = tmpl.slice(0, etm.start(offset)); + const tmplEnd = tmpl.slice(etm.end(offset)); + + const updatedTmpl = tmplStart + ifThenElseBlock + tmplEnd; + + // decrease usage count of thenTmpl and elseTmpl + thenTmpl.count--; + thenTmpl.used = true; + elseTmpl.count--; + elseTmpl.used = true; + + const pre = originals.start.length - startBlock.length; + const post = originals.end.length - postBlock.length; + + return {tmpl: updatedTmpl, offsets: {pre, post}}; +} + function migrateNgFor( - etm: ElementToMigrate, tmpl: string, offset: number): {tmpl: string, offset: number} { + etm: ElementToMigrate, tmpl: string, offset: number, hasLineBreaks: boolean): Result { const aliasWithEqualRegexp = /=\s+(count|index|first|last|even|odd)/gm; const aliasWithAsRegexp = /(count|index|first|last|even|odd)\s+as/gm; const aliases = []; - + const lbString = hasLineBreaks ? lb : ''; + const lbSpaces = hasLineBreaks ? `${lb} ` : ''; const parts = etm.attr.value.split(';'); + const originals = getOriginals(etm, tmpl, offset); + // first portion should always be the loop definition prefixed with `let` const condition = parts[0].replace('let ', ''); const loopVar = condition.split(' of ')[0]; @@ -222,148 +349,97 @@ function migrateNgFor( const aliasStr = (aliases.length > 0) ? `;${aliases.join(';')}` : ''; - const startBlock = `@for (${condition}; track ${trackBy}${aliasStr}) {`; + const {start, middle, end} = getMainBlock(etm, tmpl, offset); + const startBlock = `@for (${condition}; track ${trackBy}${aliasStr}) {${lbSpaces}${start}`; - const mainBlock = getMainBlock(etm, tmpl, offset); - const forBlock = startBlock + mainBlock + '}'; + const endBlock = `${end}${lbString}}`; + const forBlock = startBlock + middle + endBlock; const updatedTmpl = tmpl.slice(0, etm.start(offset)) + forBlock + tmpl.slice(etm.end(offset)); - offset = offset + etm.length() - forBlock.length; + const pre = originals.start.length - startBlock.length; + const post = originals.end.length - endBlock.length; - return {tmpl: updatedTmpl, offset}; + return {tmpl: updatedTmpl, offsets: {pre, post}}; } -function migrateNgIf( - etm: ElementToMigrate, ngTemplates: Map, tmpl: string, - offset: number): {tmpl: string, offset: number} { - const matchThen = etm.attr.value.match(/;\s+then/gm); - const matchElse = etm.attr.value.match(/;\s+else/gm); +function getOriginals( + etm: ElementToMigrate, tmpl: string, offset: number): {start: string, end: string} { + // original opening block + if (etm.el.children.length > 0) { + const start = tmpl.slice( + etm.el.sourceSpan.start.offset - offset, + etm.el.children[0].sourceSpan.start.offset - offset); + // original closing block + const end = tmpl.slice( + etm.el.children[etm.el.children.length - 1].sourceSpan.end.offset - offset, + etm.el.sourceSpan.end.offset - offset); + return {start, end}; + } + // self closing or no children + const start = + tmpl.slice(etm.el.sourceSpan.start.offset - offset, etm.el.sourceSpan.end.offset - offset); + // original closing block + return {start, end: ''}; +} - if (matchThen && matchThen.length > 0) { - return buildIfThenElseBlock(etm, ngTemplates, tmpl, matchThen[0], matchElse![0], offset); - } else if (matchElse && matchElse.length > 0) { - // just else - return buildIfElseBlock(etm, ngTemplates, tmpl, matchElse[0], offset); +function getMainBlock(etm: ElementToMigrate, tmpl: string, offset: number): + {start: string, middle: string, end: string} { + if (etm.el.name === 'ng-container' && etm.el.attrs.length === 1) { + // 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: ''}; } - return buildIfBlock(etm, tmpl, offset); -} + const attrStart = etm.attr.keySpan!.start.offset - 1 - offset; + const valEnd = etm.attr.valueSpan!.end.offset + 1 - offset; + let childStart = valEnd; + let childEnd = valEnd; -function buildIfBlock( - etm: ElementToMigrate, tmpl: string, offset: number): {tmpl: string, offset: number} { - // includes the mandatory semicolon before as - const condition = etm.attr.value.replace(' as ', '; as '); - - const startBlock = `@if (${condition}) {`; - - const ifBlock = startBlock + getMainBlock(etm, tmpl, offset) + `}`; - const updatedTmpl = tmpl.slice(0, etm.start(offset)) + ifBlock + tmpl.slice(etm.end(offset)); - - offset = offset + etm.length() - ifBlock.length; - - return {tmpl: updatedTmpl, offset}; -} - -function buildIfElseBlock( - etm: ElementToMigrate, ngTemplates: Map, tmpl: string, elseString: string, - offset: number): {tmpl: string, offset: number} { - // includes the mandatory semicolon before as - const condition = etm.getCondition(elseString).replace(' as ', '; as '); - - const elseTmpl = ngTemplates.get(`#${etm.getTemplateName(elseString)}`)!; - const startBlock = `@if (${condition}) {`; - const mainBlock = getMainBlock(etm, tmpl, offset); - const elseBlock = `} @else {`; - const postBlock = elseBlock + elseTmpl.children + '}'; - const ifElseBlock = startBlock + mainBlock + postBlock; - - let tmplStart = tmpl.slice(0, etm.start(offset)); - let tmplEnd = tmpl.slice(etm.end(offset)); - const updatedTmpl = tmplStart + ifElseBlock + tmplEnd; - - offset = offset + etm.preOffset(startBlock.length) + - etm.postOffset(mainBlock.length + postBlock.length); - - // decrease usage count of elseTmpl - elseTmpl.count--; - - return {tmpl: updatedTmpl, offset}; -} - -function buildIfThenElseBlock( - etm: ElementToMigrate, ngTemplates: Map, tmpl: string, thenString: string, - elseString: string, offset: number): {tmpl: string, offset: number} { - const condition = etm.getCondition(thenString).replace(' as ', '; as '); - - const startBlock = `@if (${condition}) {`; - const elseBlock = `} @else {`; - - const thenTmpl = ngTemplates.get(`#${etm.getTemplateName(thenString, elseString)}`)!; - const elseTmpl = ngTemplates.get(`#${etm.getTemplateName(elseString)}`)!; - - const postBlock = thenTmpl.children + elseBlock + elseTmpl.children + '}'; - const ifThenElseBlock = startBlock + postBlock; - - let tmplStart = tmpl.slice(0, etm.start(offset)); - let tmplEnd = tmpl.slice(etm.end(offset)); - - const updatedTmpl = tmplStart + ifThenElseBlock + tmplEnd; - - offset = offset + etm.preOffset(startBlock.length) + etm.postOffset(postBlock.length); - - // decrease usage count of thenTmpl and elseTmpl - thenTmpl.count--; - elseTmpl.count--; - - return {tmpl: updatedTmpl, offset}; -} - -function getMainBlock(etm: ElementToMigrate, tmpl: string, offset: number) { - if (etm.el.name === 'ng-container' && etm.el.attrs.length === 1 && etm.attr.name === ngfor) { - // this is the case where we're migrating an ngFor and there's no need to keep the ng-container - const childStart = etm.el.children[0].sourceSpan.start.offset - etm.nestCount - offset; - const childEnd = - etm.el.children[etm.el.children.length - 1].sourceSpan.end.offset - etm.nestCount - offset; - return tmpl.slice(childStart, childEnd); + 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; } - const attrStart = etm.attr.keySpan!.start.offset - 1 - etm.nestCount - offset; - const valEnd = etm.attr.valueSpan!.end.offset + 1 - etm.nestCount - offset; - const start = tmpl.slice(etm.start(offset), attrStart); - const end = tmpl.slice(valEnd, etm.end(offset)); - return start + end; + + let start = tmpl.slice(etm.start(offset), attrStart); + start += tmpl.slice(valEnd, childStart); + const middle = tmpl.slice(childStart, childEnd); + const end = tmpl.slice(childEnd, etm.end(offset)); + + return {start, middle, end}; } function migrateNgSwitch( - etm: ElementToMigrate, tmpl: string, offset: number): {tmpl: string, offset: number} { + etm: ElementToMigrate, tmpl: string, offset: number, hasLineBreaks: boolean): Result { const condition = etm.attr.value; - const startBlock = `@switch (${condition}) { `; + const startBlock = `@switch (${condition}) {`; + const lbString = hasLineBreaks ? lb : ''; const {openTag, closeTag, children} = getSwitchBlockElements(etm, tmpl, offset); - const cases = getSwitchCases(children, tmpl, etm.nestCount, offset); - const switchBlock = openTag + startBlock + cases.join(' ') + `}` + closeTag; + const cases = getSwitchCases(children, tmpl, offset, hasLineBreaks); + const switchBlock = openTag + startBlock + cases.join('') + `${lbString}}` + closeTag; const updatedTmpl = tmpl.slice(0, etm.start(offset)) + switchBlock + tmpl.slice(etm.end(offset)); + const pre = etm.length() - switchBlock.length; - const difference = etm.length() - switchBlock.length; - - offset = offset + difference; - - return {tmpl: updatedTmpl, offset}; + return {tmpl: updatedTmpl, offsets: {pre, post: 0}}; } function getSwitchBlockElements(etm: ElementToMigrate, tmpl: string, offset: number) { - const attrStart = etm.attr.keySpan!.start.offset - 1 - etm.nestCount - offset; - const valEnd = etm.attr.valueSpan!.end.offset + 1 - etm.nestCount - offset; - const childStart = etm.el.children[0].sourceSpan.start.offset - etm.nestCount - offset; - const childEnd = - etm.el.children[etm.el.children.length - 1].sourceSpan.end.offset - etm.nestCount - offset; - let openTag = tmpl.slice(etm.start(offset), attrStart) + tmpl.slice(valEnd, childStart); - if (tmpl.slice(childStart, childStart + 1) === '\n') { - openTag += '\n'; + const attrStart = etm.attr.keySpan!.start.offset - 1 - offset; + const valEnd = etm.attr.valueSpan!.end.offset + 1 - offset; + const childStart = etm.el.children[0].sourceSpan.start.offset - offset; + const childEnd = etm.el.children[etm.el.children.length - 1].sourceSpan.end.offset - offset; + let openTag = (etm.el.name === 'ng-container') ? + '' : + tmpl.slice(etm.start(offset), attrStart) + tmpl.slice(valEnd, childStart); + if (tmpl.slice(childStart, childStart + 1) === lb) { + openTag += lb; } - let closeTag = tmpl.slice(childEnd, etm.end(offset)); - if (tmpl.slice(childEnd - 1, childEnd) === '\n') { - closeTag = '\n' + closeTag; + let closeTag = (etm.el.name === 'ng-container') ? '' : tmpl.slice(childEnd, etm.end(offset)); + if (tmpl.slice(childEnd - 1, childEnd) === lb) { + closeTag = lb + closeTag; } return { openTag, @@ -372,25 +448,38 @@ function getSwitchBlockElements(etm: ElementToMigrate, tmpl: string, offset: num }; } -function getSwitchCases(children: Node[], tmpl: string, nestCount: number, offset: number) { +function getSwitchCases(children: Node[], tmpl: string, offset: number, hasLineBreaks: boolean) { const collector = new CaseCollector(); visitAll(collector, children); - return collector.elements.map(etm => getSwitchCaseBlock(etm, tmpl, nestCount, offset)); + return collector.elements.map(etm => getSwitchCaseBlock(etm, tmpl, offset, hasLineBreaks)); } function getSwitchCaseBlock( - etm: ElementToMigrate, tmpl: string, nestCount: number, offset: number): string { - const elStart = etm.el.sourceSpan?.start.offset - nestCount - offset; - const elEnd = etm.el.sourceSpan?.end.offset - nestCount - offset; - // beginning of the ngIf minus a leading space - const attrStart = etm.attr.keySpan!.start.offset - 1 - nestCount - offset; + etm: ElementToMigrate, tmpl: string, offset: number, hasLineBreaks: boolean): string { + let elStart = etm.el.sourceSpan?.start.offset - offset; + let elEnd = etm.el.sourceSpan?.end.offset - offset; + const lbString = hasLineBreaks ? '\n ' : ' '; + const lbSpaces = hasLineBreaks ? ' ' : ''; + let shift = 0; + + if ((etm.el.name === 'ng-container' || etm.el.name === 'ng-template') && + etm.el.attrs.length === 1) { + // no need to keep the containers + elStart = etm.el.children[0].sourceSpan.start.offset - offset; + elEnd = etm.el.children[etm.el.children.length - 1].sourceSpan.end.offset - offset; + // account for the `>` that isn't needed + shift += 1; + } + + const attrStart = etm.attr.keySpan!.start.offset - 1 - offset + shift; // ngSwitchDefault case has no valueSpan and relies on the end of the key - const attrEnd = etm.attr.keySpan!.end.offset - nestCount - offset; - if (etm.attr.name === '*ngSwitchDefault') { - return `@default { ${tmpl.slice(elStart, attrStart) + tmpl.slice(attrEnd, elEnd)} }`; + if (etm.attr.name === '*ngSwitchDefault' || etm.attr.name === 'ngSwitchDefault') { + const attrEnd = etm.attr.keySpan!.end.offset - offset + shift; + return `${lbString}@default {${lbString}${lbSpaces}${ + tmpl.slice(elStart, attrStart) + tmpl.slice(attrEnd, elEnd)}${lbString}}`; } // ngSwitchCase has a valueSpan - const valEnd = etm.attr.valueSpan!.end.offset + 1 - nestCount - offset; - return `@case (${etm.attr.value}) { ${ - tmpl.slice(elStart, attrStart) + tmpl.slice(valEnd, elEnd)} }`; + let valEnd = etm.attr.valueSpan!.end.offset + 1 - offset + shift; + return `${lbString}@case (${etm.attr.value}) {${lbString}${lbSpaces}${ + tmpl.slice(elStart, attrStart) + tmpl.slice(valEnd, elEnd)}${lbString}}`; } diff --git a/packages/core/schematics/test/control_flow_migration_spec.ts b/packages/core/schematics/test/control_flow_migration_spec.ts index 4e42c396f0c..7133351b747 100644 --- a/packages/core/schematics/test/control_flow_migration_spec.ts +++ b/packages/core/schematics/test/control_flow_migration_spec.ts @@ -139,7 +139,9 @@ describe('control flow migration', () => { expect(content).toBe([ `
`, - `@if (show) {Content here}`, + `@if (show) {`, + `Content here`, + `}`, `
`, ].join('\n')); }); @@ -176,7 +178,102 @@ describe('control flow migration', () => { expect(content).toBe([ `
`, - `@if (show) {Content here}`, + `@if (show) {`, + `Content here`, + `}`, + `
`, + ].join('\n')); + }); + + it('should migrate an if case with no star', 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', [ + `
`, + `Content here`, + `
`, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe([ + `
`, + `@if (show) {`, + `Content here`, + `}`, + `
`, + ].join('\n')); + }); + + it('should migrate an if case as a binding', 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', [ + `
`, + `Content here`, + `
`, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe([ + `
`, + `@if (show) {`, + `Content here`, + `}`, + `
`, + ].join('\n')); + }); + + it('should migrate an if case on a container', 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', [ + `
`, + `Content here`, + `
`, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe([ + `
`, + `@if (show) {`, + `Content here`, + `}`, `
`, ].join('\n')); }); @@ -206,7 +303,11 @@ describe('control flow migration', () => { expect(content).toBe([ `
`, - `@if (show) {Content here} @else {Else Content}`, + `@if (show) {`, + `Content here`, + `} @else {`, + `Else Content`, + `}`, `
`, ].join('\n')); }); @@ -236,7 +337,11 @@ describe('control flow migration', () => { expect(content).toBe([ `
`, - `@if (show) {Content here} @else {Else Content}`, + `@if (show) {`, + `Content here`, + `} @else {`, + `Else Content`, + `}`, `
`, ].join('\n')); }); @@ -267,7 +372,11 @@ describe('control flow migration', () => { expect(content).toBe([ `
`, - `@if (show) {
THEN Stuff
} @else {Else Content}`, + `@if (show) {`, + `
THEN Stuff
`, + `} @else {`, + `Else Content`, + `}`, `
`, ].join('\n')); }); @@ -298,7 +407,11 @@ describe('control flow migration', () => { expect(content).toBe([ `
`, - `@if (show) {
THEN Stuff
} @else {Else Content}`, + `@if (show) {`, + `
THEN Stuff
`, + `} @else {`, + `Else Content`, + `}`, `
`, ].join('\n')); }); @@ -330,7 +443,11 @@ describe('control flow migration', () => { expect(content).toBe([ `
`, - `@if (show) {
THEN Stuff
} @else {Else Content}`, + `@if (show) {`, + `
THEN Stuff
`, + `} @else {`, + `Else Content`, + `}`, `Else Content`, `
`, ``, @@ -377,14 +494,12 @@ describe('control flow migration', () => { } `); - writeFile( - '/comp.html', [`
{{ user.name }}
`].join('\n')); + writeFile('/comp.html', `
{{ user.name }}
`); await runMigration(); const content = tree.readContent('/comp.html'); - expect(content).toBe( - [`@if (user$ | async; as user) {
{{ user.name }}
}`].join('\n')); + expect(content).toBe(`@if (user$ | async; as user) {
{{ user.name }}
}`); }); it('should migrate if/else with alias', async () => { @@ -412,7 +527,11 @@ describe('control flow migration', () => { expect(content).toBe([ `
`, - `@if (user$ | async; as user) {
{{ user.name }}
} @else {No user}`, + `@if (user$ | async; as user) {`, + `
{{ user.name }}
`, + `} @else {`, + `No user`, + `}`, `
`, ].join('\n')); }); @@ -443,7 +562,11 @@ describe('control flow migration', () => { expect(content).toBe([ `
`, - `@if (user$ | async; as user) {User} @else {No user}`, + `@if (user$ | async; as user) {`, + `User`, + `} @else {`, + `No user`, + `}`, `
`, ].join('\n')); }); @@ -539,7 +662,9 @@ describe('control flow migration', () => { expect(content).toBe([ `
    `, - `@for (item of items; track item) {
  • {{item.text}}
    • }`, + `@for (item of items; track item) {`, + `
  • {{item.text}}
    • `, + `}`, `
`, ].join('\n')); }); @@ -588,7 +713,9 @@ describe('control flow migration', () => { expect(content).toBe([ `
    `, - `@for (item of items; track item) {
  • {{item.text}}
    • }`, + `@for (item of items; track item) {`, + `
  • {{item.text}}
    • `, + `}`, `
`, ].join('\n')); }); @@ -768,7 +895,7 @@ describe('control flow migration', () => { const content = tree.readContent('/comp.ts'); expect(content).toContain( - 'template: `
@switch (testOpts) { @case (1) {

Option 1

} @case (2) {

Option 2

}}
`'); + 'template: `
@switch (testOpts) { @case (1) {

Option 1

} @case (2) {

Option 2

}}
'); }); it('should migrate multiple inline templates in the same file', async () => { @@ -833,7 +960,17 @@ describe('control flow migration', () => { const content = tree.readContent('/comp.html'); expect(content).toBe([ `
`, - `@switch (testOpts) { @case (1) {

Option 1

} @case (2) {

Option 2

} @default {

Option 3

}}`, + `@switch (testOpts) {`, + ` @case (1) {`, + `

Option 1

`, + ` }`, + ` @case (2) {`, + `

Option 2

`, + ` }`, + ` @default {`, + `

Option 3

`, + ` }`, + `}`, `
`, ].join('\n')); }); @@ -876,13 +1013,158 @@ describe('control flow migration', () => { expect(content).toBe([ `
`, - `@switch (testOpts) { @case (1) {

Option 1

} @case (2) {

Option 2

} @default {

Option 3

}}`, + `@switch (testOpts) {`, + ` @case (1) {`, + `

Option 1

`, + ` }`, + ` @case (2) {`, + `

Option 2

`, + ` }`, + ` @default {`, + `

Option 3

`, + ` }`, + `}`, `
`, ].join('\n')); }); + + it('should remove unnecessary ng-containers', async () => { + writeFile( + '/comp.ts', + ` + import {Component} from '@angular/core'; + import {ngSwitch, ngSwitchCase} from '@angular/common'; + + @Component({ + template: \`
` + + `

Option 1

` + + `

Option 2

` + + `
\` + }) + class Comp { + testOpts = "1"; + } + `); + + await runMigration(); + const content = tree.readContent('/comp.ts'); + + expect(content).toContain( + 'template: `
@switch (testOpts) { @case (1) {

Option 1

} @case (2) {

Option 2

}}
'); + }); + + it('should remove unnecessary ng-container on ngswitch', async () => { + writeFile( + '/comp.ts', + ` + import {Component} from '@angular/core'; + import {ngSwitch, ngSwitchCase} from '@angular/common'; + + @Component({ + template: \`
` + + `` + + `

Option 1

` + + `

Option 2

` + + `` + + `
\` + }) + class Comp { + testOpts = "1"; + } + `); + + await runMigration(); + const content = tree.readContent('/comp.ts'); + + expect(content).toContain( + 'template: `
@switch (testOpts) { @case (1) {

Option 1

} @case (2) {

Option 2

}}
'); + }); + + it('should handle cases with missing star', async () => { + writeFile( + '/comp.ts', + ` + import {Component} from '@angular/core'; + import {ngSwitch, ngSwitchCase} from '@angular/common'; + + @Component({ + template: \`
` + + `

Option 1

` + + `

Option 2

` + + `

Option 3

` + + `
\` + }) + class Comp { + testOpts = "1"; + } + `); + + await runMigration(); + const content = tree.readContent('/comp.ts'); + + expect(content).toContain( + 'template: `
@switch (testOpts) { @case (1) {

Option 1

} @case (2) {

Option 2

} @default {

Option 3

}}
'); + }); + + it('should handle cases with binding', async () => { + writeFile( + '/comp.ts', + ` + import {Component} from '@angular/core'; + import {ngSwitch, ngSwitchCase} from '@angular/common'; + + @Component({ + template: \`
` + + `

Option 1

` + + `

Option 2

` + + `

Option 3

` + + `
\` + }) + class Comp { + testOpts = "1"; + } + `); + + await runMigration(); + const content = tree.readContent('/comp.ts'); + + expect(content).toContain( + 'template: `
@switch (testOpts) { @case (1) {

Option 1

} @case (2) {

Option 2

} @default {

Option 3

}}
'); + }); }); describe('nested structures', () => { + it('should migrate an inline template with nested control flow structures and no line breaks', + async () => { + writeFile('/comp.ts', ` + import {Component} from '@angular/core'; + import {NgIf} from '@angular/common'; + + @Component({ + imports: [NgFor, NgIf], + templateUrl: './comp.html' + }) + class Comp { + show = false; + nest = true; + again = true; + more = true; + } + `); + + writeFile( + '/comp.html', + `
thingsstuff
stuff
`, + ); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe( + `@if (show) {
@if (nest) {
@if (again) {things}@if (more) {stuff}
}@if (more) {stuff}
}`, + ); + }); + it('should migrate an inline template with multiple nested control flow structures', async () => { writeFile('/comp.ts', ` @@ -903,11 +1185,10 @@ describe('control flow migration', () => { writeFile('/comp.html', [ `
`, + `things`, `
`, - `things`, - `stuff`, + `stuff`, `
`, - `stuff`, `
`, ].join('\n')); @@ -915,16 +1196,222 @@ describe('control flow migration', () => { const content = tree.readContent('/comp.html'); expect(content).toBe([ - `@if (show) {
`, - `@if (nest) {
`, - `@if (again) {things}`, - `@if (more) {stuff}`, - `
}`, - `@if (more) {stuff}`, - `
}`, + `@if (show) {`, + `
`, + `things`, + `@if (nest) {`, + `
`, + `stuff`, + `
`, + `}`, + `
`, + `}`, ].join('\n')); }); + it('should migrate an inline template with multiple nested control flow structures', + async () => { + writeFile('/comp.ts', ` + import {Component} from '@angular/core'; + import {NgIf} from '@angular/common'; + + @Component({ + imports: [NgFor, NgIf], + templateUrl: './comp.html' + }) + class Comp { + show = false; + nest = true; + again = true; + more = true; + } + `); + + writeFile('/comp.html', [ + `
`, + `things`, + `
`, + `stuff`, + `
`, + `
`, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe([ + `@if (show) {`, + `
`, + `things`, + `@if (nest) {`, + `
`, + `stuff`, + `
`, + `}`, + `
`, + `}`, + ].join('\n')); + }); + + it('should migrate a simple nested case', async () => { + writeFile('/comp.ts', ` + import {Component} from '@angular/core'; + import {NgIf} from '@angular/common'; + + @Component({ + imports: [NgFor, NgIf], + templateUrl: './comp.html' + }) + class Comp { + show = false; + nest = true; + again = true; + more = true; + } + `); + + writeFile('/comp.html', [ + `
`, + `
`, + `
`, + `
`, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe([ + `@if (show) {`, + `
`, + `@if (nest) {`, + `
`, + `
`, + `}`, + `
`, + `}`, + ].join('\n')); + }); + + it('should migrate a simple for inside an if', async () => { + writeFile('/comp.ts', ` + import {Component} from '@angular/core'; + import {NgIf} from '@angular/common'; + + @Component({ + imports: [NgFor, NgIf], + templateUrl: './comp.html' + }) + class Comp { + show = false; + nest = true; + again = true; + more = true; + } + `); + + writeFile('/comp.html', [ + `
    `, + `
  • {{h.name}} ({{h.emotion}})
    • `, + `
`, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe([ + `@if (show) {`, + `
    `, + `@for (h of heroes; track h) {`, + `
  • {{h.name}} ({{h.emotion}})
    • `, + `}`, + `
`, + `}`, + ].join('\n')); + }); + + it('should migrate an if inside a for loop', async () => { + writeFile('/comp.ts', ` + import {Component} from '@angular/core'; + import {NgIf} from '@angular/common'; + + @Component({ + imports: [NgFor, NgIf], + templateUrl: './comp.html' + }) + class Comp { + heroes = [{name: 'cheese', emotion: 'happy'},{name: 'stuff', emotion: 'sad'}]; + show = true; + } + `); + + writeFile('/comp.html', [ + ``, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe([ + ``, + ].join('\n')); + }); + + it('should migrate an if inside a for loop with ng-containers', async () => { + writeFile('/comp.ts', ` + import {Component} from '@angular/core'; + import {NgIf} from '@angular/common'; + + @Component({ + imports: [NgFor, NgIf], + templateUrl: './comp.html' + }) + class Comp { + heroes = [{name: 'cheese', emotion: 'happy'},{name: 'stuff', emotion: 'sad'}]; + show = true; + } + `); + + writeFile('/comp.html', [ + ``, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + + expect(content).toBe([ + ``, + ].join('\n')); + }); + it('should migrate an inline template with if and for loops', async () => { writeFile('/comp.ts', ` import {Component} from '@angular/core'; @@ -957,15 +1444,21 @@ describe('control flow migration', () => { const content = tree.readContent('/comp.html'); expect(content).toBe([ - `@if (show) {
`, - `@if (nest) {
    `, - `@for (item of items; track item) {
  • {{item.text}}
    • }`, - `
}`, - `
}`, + `@if (show) {`, + `
`, + `@if (nest) {`, + `
    `, + `@for (item of items; track item) {`, + `
  • {{item.text}}
    • `, + `}`, + `
`, + `}`, + `
`, + `}`, ].join('\n')); }); - it('should migrate an inline template with if, else and for loops', async () => { + it('should migrate template with if, else, and for loops', async () => { writeFile('/comp.ts', ` import {Component} from '@angular/core'; import {NgIf, NgFor} from '@angular/common'; @@ -998,52 +1491,19 @@ describe('control flow migration', () => { const content = tree.readContent('/comp.html'); expect(content).toBe([ - `@if (show) {
`, - `@if (nest) {
    `, - `@for (item of items; track item) {
  • {{item.text}}
    • }`, - `
} @else {

Else content

}`, - `
}`, - ].join('\n')); - }); - - it('should migrate an inline template with if, else, and for loops', async () => { - writeFile('/comp.ts', ` - import {Component} from '@angular/core'; - import {NgIf, NgFor} from '@angular/common'; - interface Item { - id: number; - text: string; - } - - @Component({ - imports: [NgFor, NgIf], - templateUrl: './comp.html' - }) - class Comp { - show = false; - nest = true; - items: Item[] = [{id: 1, text: 'blah'},{id: 2, text: 'stuff'}]; - } - `); - - writeFile('/comp.html', [ - `
`, - `
    `, - `
  • {{item.text}}
    • `, + `@if (show) {`, + `
    `, + `@if (nest) {`, + `
      `, + `@for (item of items; track item) {`, + `
    • {{item.text}}
      • `, + `}`, `
    `, - `

    Else content

    `, + `} @else {`, + `

    Else content

    `, + `}`, `
    `, - ].join('\n')); - - await runMigration(); - const content = tree.readContent('/comp.html'); - - expect(content).toBe([ - `@if (show) {
    `, - `@if (nest) {
      `, - `@for (item of items; track item) {
    • {{item.text}}
      • }`, - `
    } @else {

    Else content

    }`, - `
    }`, + `}`, ].join('\n')); }); @@ -1091,21 +1551,231 @@ describe('control flow migration', () => { const content = tree.readContent('/comp.html'); expect(content).toBe([ - `@if (show) {
    `, - `@if (again) {
    `, - `@if (more) {
    `, + `@if (show) {`, `
    `, - `@switch (testOpts) { @case (1) {

    Option 1

    } @case (2) {

    Option 2

    } @default {

    Option 3

    }}`, + `@if (again) {`, + `
    `, + `@if (more) {`, + `
    `, + `
    `, + `@switch (testOpts) {`, + ` @case (1) {`, + `

    Option 1

    `, + ` }`, + ` @case (2) {`, + `

    Option 2

    `, + ` }`, + ` @default {`, + `

    Option 3

    `, + ` }`, + `}`, `
    `, - `
    }`, - `
    }`, - `@if (nest) {
      `, - `@for (item of items; track item) {
    • {{item.text}}
      • }`, - `
    }`, - `
    }`, + `
    `, + `}`, + `
    `, + `}`, + `@if (nest) {`, + `
      `, + `@for (item of items; track item) {`, + `
    • {{item.text}}
      • `, + `}`, + `
    `, + `}`, + `
    `, + `}`, ].join('\n')); }); + + it('complicated case', async () => { + writeFile('/comp.ts', ` + import {Component} from '@angular/core'; + import {NgIf, NgFor} from '@angular/common'; + interface Item { + id: number; + text: string; + } + + @Component({ + imports: [NgFor, NgIf], + templateUrl: './comp.html' + }) + class Comp { + } + `); + + writeFile('/comp.html', [ + `
    `, + ``, + ``, + `{{ breadcrumb.label }}`, + ``, + ``, + `{{ breadcrumb.label }}`, + ``, + ``, + ``, + `{{ breadcrumb.label }}`, + ``, + `
    `, + ].join('\n')); + + await runMigration(); + const content = tree.readContent('/comp.html'); + const result = [ + `@for (breadcrumb of breadcrumbItems(); track breadcrumb) {`, + `
    `, + `@if (breadcrumb.path) {\n`, + `@if (breadcrumb.isExternal) {\n`, + `{{ breadcrumb.label }}\n`, + `} @else {\n`, + `{{ breadcrumb.label }}\n`, + `}\n`, + `} @else {\n`, + `{{ breadcrumb.label }}\n`, + `}`, + `
    `, + `}`, + ].join('\n'); + + expect(content).toBe(result); + }); + + it('long file', async () => { + writeFile('/comp.ts', ` + import {Component} from '@angular/core'; + import {NgIf, NgFor} from '@angular/common'; + interface Item { + id: number; + text: string; + } + + @Component({ + imports: [NgFor, NgIf], + templateUrl: './comp.html' + }) + class Comp { + } + `); + + writeFile('/comp.html', [ + `
    `, + ``, + ``, + ``, + ``, + `
    `, + ``, + ``, + `Case A`, + ``, + ``, + `Case B`, + ``, + ``, + `Case C`, + ``, + ``, + `Case D`, + ``, + ``, + `Case E`, + ``, + ``, + ``, + `
    `, + `
    `, + ].join('\n')); + + await runMigration(); + + const content = tree.readContent('/comp.html'); + const result = [ + `
    `, + `@if (stuff) {`, + ``, + `}`, + `@if (shouldDoIt) {\n`, + `\n`, + `}`, + `@if (`, + `shouldWhat`, + `) {`, + `
    \n`, + `@switch (currentCase()) {`, + ` @case (A) {`, + ` `, + `Case A`, + ``, + ` }`, + ` @case (B) {`, + ` `, + `Case B`, + ``, + ` }`, + ` @case (C) {`, + ` `, + `Case C`, + ``, + ` }`, + ` @case (D) {`, + ` `, + `Case D`, + ``, + ` }`, + ` @case (E) {`, + ` `, + `Case E`, + ``, + ` }`, + `}\n`, + ``, + `
    `, + `}`, + `
    `, + ].join('\n'); + + expect(content).toBe(result); + }); }); + describe('error handling', () => { it('should log template migration errors to the console', async () => { writeFile('/comp.ts', ` @@ -1127,4 +1797,32 @@ describe('control flow migration', () => { expect(warnOutput.join(' ')).toContain('WARNING: 1 errors occured during your migration'); }); }); + + describe('template removal', () => { + it('should not remove a template thats not used in control flow', async () => { + writeFile('/comp.ts', ` + import {Component} from '@angular/core'; + import {NgIf} from '@angular/common'; + + @Component({ + selector: 'declare-comp', + template: \` + DeclareComp({{name}}) + + {{greeting}} {{logName()}}! + + \` + }) + class DeclareComp implements DoCheck, AfterViewChecked { + @ViewChild('myTmpl') myTmpl!: TemplateRef; + name: string = 'world'; + } + `); + + await runMigration(); + const content = tree.readContent('/comp.ts'); + + expect(content).toContain(''); + }); + }); });