Skip to content

Commit bf4d856

Browse files
crisbetopkozlowski-opensource
crisbeto
authored and
pkozlowski-opensource
committed
fix(migrations): preserve trailing commas in code generated by standalone migration (#49533)
This is based on some internal feedback. Adds logic to the standalone migration that attempts to preserve trailing commas when updating existing AST nodes. When creating new ones, it tries to infer whether to generate the trailing comma based on the surrounding code. PR Close #49533
1 parent 3bb7676 commit bf4d856

File tree

4 files changed

+208
-11
lines changed

4 files changed

+208
-11
lines changed

‎packages/core/schematics/ng-generate/standalone-migration/prune-modules.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -152,7 +152,10 @@ function removeArrayReferences(
152152
tracker: ChangeTracker): void {
153153
for (const [array, toRemove] of locations.getEntries()) {
154154
const newElements = filterRemovedElements(array.elements, toRemove);
155-
tracker.replaceNode(array, ts.factory.updateArrayLiteralExpression(array, newElements));
155+
tracker.replaceNode(
156+
array,
157+
ts.factory.updateArrayLiteralExpression(
158+
array, ts.factory.createNodeArray(newElements, array.elements.hasTrailingComma)));
156159
}
157160
}
158161

‎packages/core/schematics/ng-generate/standalone-migration/standalone-bootstrap.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -244,9 +244,13 @@ function replaceBootstrapCallExpression(
244244

245245
// Push the providers after `importProvidersFrom` call for better readability.
246246
combinedProviders.push(...providers);
247+
248+
const providersArray = ts.factory.createNodeArray(
249+
combinedProviders,
250+
analysis.metadata.properties.hasTrailingComma && combinedProviders.length > 2);
247251
const initializer = remapDynamicImports(
248252
sourceFile.fileName,
249-
ts.factory.createArrayLiteralExpression(combinedProviders, combinedProviders.length > 1));
253+
ts.factory.createArrayLiteralExpression(providersArray, combinedProviders.length > 1));
250254

251255
args.push(ts.factory.createObjectLiteralExpression(
252256
[ts.factory.createPropertyAssignment('providers', initializer)], true));

‎packages/core/schematics/ng-generate/standalone-migration/to-standalone.ts‎

Lines changed: 40 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -98,10 +98,15 @@ export function convertNgModuleDeclarationToStandalone(
9898
decl, allDeclarations, tracker, typeChecker, importRemapper);
9999

100100
if (importsToAdd.length > 0) {
101+
const hasTrailingComma = importsToAdd.length > 2 &&
102+
!!extractMetadataLiteral(directiveMeta.decorator)?.properties.hasTrailingComma;
101103
decorator = addPropertyToAngularDecorator(
102104
decorator,
103105
ts.factory.createPropertyAssignment(
104-
'imports', ts.factory.createArrayLiteralExpression(importsToAdd)));
106+
'imports',
107+
ts.factory.createArrayLiteralExpression(
108+
// Create a multi-line array when it has a trailing comma.
109+
ts.factory.createNodeArray(importsToAdd, hasTrailingComma), hasTrailingComma)));
105110
}
106111
}
107112

@@ -215,6 +220,9 @@ function moveDeclarationsToImports(
215220
const declarationsToCopy: ts.Expression[] = [];
216221
const properties: ts.ObjectLiteralElementLike[] = [];
217222
const importsProp = findLiteralProperty(literal, 'imports');
223+
const hasAnyArrayTrailingComma = literal.properties.some(
224+
prop => ts.isPropertyAssignment(prop) && ts.isArrayLiteralExpression(prop.initializer) &&
225+
prop.initializer.elements.hasTrailingComma);
218226

219227
// Separate the declarations that we want to keep and ones we need to copy into the `imports`.
220228
if (ts.isPropertyAssignment(declarationsProp)) {
@@ -248,7 +256,9 @@ function moveDeclarationsToImports(
248256
// If there are no `imports`, create them with the declarations we want to copy.
249257
if (!importsProp && declarationsToCopy.length > 0) {
250258
properties.push(ts.factory.createPropertyAssignment(
251-
'imports', ts.factory.createArrayLiteralExpression(declarationsToCopy)));
259+
'imports',
260+
ts.factory.createArrayLiteralExpression(ts.factory.createNodeArray(
261+
declarationsToCopy, hasAnyArrayTrailingComma && declarationsToCopy.length > 2))));
252262
}
253263

254264
for (const prop of literal.properties) {
@@ -260,8 +270,13 @@ function moveDeclarationsToImports(
260270
// If we have declarations to preserve, update the existing property, otherwise drop it.
261271
if (prop === declarationsProp) {
262272
if (declarationsToPreserve.length > 0) {
273+
const hasTrailingComma = ts.isArrayLiteralExpression(prop.initializer) ?
274+
prop.initializer.elements.hasTrailingComma :
275+
hasAnyArrayTrailingComma;
263276
properties.push(ts.factory.updatePropertyAssignment(
264-
prop, prop.name, ts.factory.createArrayLiteralExpression(declarationsToPreserve)));
277+
prop, prop.name,
278+
ts.factory.createArrayLiteralExpression(ts.factory.createNodeArray(
279+
declarationsToPreserve, hasTrailingComma && declarationsToPreserve.length > 2))));
265280
}
266281
continue;
267282
}
@@ -273,10 +288,16 @@ function moveDeclarationsToImports(
273288

274289
if (ts.isArrayLiteralExpression(prop.initializer)) {
275290
initializer = ts.factory.updateArrayLiteralExpression(
276-
prop.initializer, [...prop.initializer.elements, ...declarationsToCopy]);
291+
prop.initializer,
292+
ts.factory.createNodeArray(
293+
[...prop.initializer.elements, ...declarationsToCopy],
294+
prop.initializer.elements.hasTrailingComma));
277295
} else {
278-
initializer = ts.factory.createArrayLiteralExpression(
279-
[ts.factory.createSpreadElement(prop.initializer), ...declarationsToCopy]);
296+
initializer = ts.factory.createArrayLiteralExpression(ts.factory.createNodeArray(
297+
[ts.factory.createSpreadElement(prop.initializer), ...declarationsToCopy],
298+
// Expect the declarations to be greater than 1 since
299+
// we have the pre-existing initializer already.
300+
hasAnyArrayTrailingComma && declarationsToCopy.length > 1));
280301
}
281302

282303
properties.push(ts.factory.updatePropertyAssignment(prop, prop.name, initializer));
@@ -288,7 +309,9 @@ function moveDeclarationsToImports(
288309
}
289310

290311
tracker.replaceNode(
291-
literal, ts.factory.updateObjectLiteralExpression(literal, properties),
312+
literal,
313+
ts.factory.updateObjectLiteralExpression(
314+
literal, ts.factory.createNodeArray(properties, literal.properties.hasTrailingComma)),
292315
ts.EmitHint.Expression);
293316
}
294317

@@ -313,10 +336,12 @@ function addPropertyToAngularDecorator(
313336
}
314337

315338
let literalProperties: ts.ObjectLiteralElementLike[];
339+
let hasTrailingComma = false;
316340

317341
if (node.expression.arguments.length === 0) {
318342
literalProperties = [property];
319343
} else if (ts.isObjectLiteralExpression(node.expression.arguments[0])) {
344+
hasTrailingComma = node.expression.arguments[0].properties.hasTrailingComma;
320345
literalProperties = [...node.expression.arguments[0].properties, property];
321346
} else {
322347
// Unsupported case (e.g. `@Component(SOME_CONST)`). Return the original node.
@@ -327,7 +352,9 @@ function addPropertyToAngularDecorator(
327352
// the latter ends up duplicating the node's leading comment.
328353
return ts.factory.createDecorator(ts.factory.createCallExpression(
329354
node.expression.expression, node.expression.typeArguments,
330-
[ts.factory.createObjectLiteralExpression(literalProperties, literalProperties.length > 1)]));
355+
[ts.factory.createObjectLiteralExpression(
356+
ts.factory.createNodeArray(literalProperties, hasTrailingComma),
357+
literalProperties.length > 1)]));
331358
}
332359

333360
/** Checks if a node is a `PropertyAssignment` with a name. */
@@ -559,12 +586,16 @@ export function migrateTestDeclarations(
559586
}
560587

561588
if (importsToAdd && importsToAdd.size > 0) {
589+
const hasTrailingComma = importsToAdd.size > 2 &&
590+
!!extractMetadataLiteral(decorator.node)?.properties.hasTrailingComma;
591+
const importsArray = ts.factory.createNodeArray(Array.from(importsToAdd), hasTrailingComma);
592+
562593
tracker.replaceNode(
563594
decorator.node,
564595
addPropertyToAngularDecorator(
565596
newDecorator,
566597
ts.factory.createPropertyAssignment(
567-
'imports', ts.factory.createArrayLiteralExpression(Array.from(importsToAdd)))));
598+
'imports', ts.factory.createArrayLiteralExpression(importsArray))));
568599
} else {
569600
tracker.replaceNode(decorator.node, newDecorator);
570601
}

‎packages/core/schematics/test/standalone_migration_spec.ts‎

Lines changed: 159 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1657,6 +1657,128 @@ describe('standalone migration', () => {
16571657
`));
16581658
});
16591659

1660+
it('should preserve the trailing comma when adding an `imports` array', async () => {
1661+
writeFile('module.ts', `
1662+
import {NgModule, Directive} from '@angular/core';
1663+
1664+
@Directive({selector: '[dir]'})
1665+
export class MyDir {}
1666+
1667+
@NgModule({
1668+
declarations: [MyDir],
1669+
exports: [MyDir],
1670+
})
1671+
export class Mod {}
1672+
`);
1673+
1674+
await runMigration('convert-to-standalone');
1675+
1676+
expect(stripWhitespace(tree.readContent('module.ts'))).toContain(stripWhitespace(`
1677+
@NgModule({
1678+
imports: [MyDir],
1679+
exports: [MyDir],
1680+
})
1681+
`));
1682+
});
1683+
1684+
it('should preserve the trailing comma when adding to an existing `imports` array', async () => {
1685+
writeFile('module.ts', `
1686+
import {NgModule, Directive} from '@angular/core';
1687+
import {CommonModule} from '@angular/common';
1688+
import {RouterModule} from '@angular/router';
1689+
1690+
@Directive({selector: '[dir]'})
1691+
export class MyDir {}
1692+
1693+
@NgModule({
1694+
imports: [
1695+
CommonModule,
1696+
RouterModule,
1697+
],
1698+
declarations: [MyDir],
1699+
exports: [MyDir],
1700+
})
1701+
export class Mod {}
1702+
`);
1703+
1704+
await runMigration('convert-to-standalone');
1705+
1706+
expect(stripWhitespace(tree.readContent('module.ts'))).toContain(stripWhitespace(`
1707+
@NgModule({
1708+
imports: [
1709+
CommonModule,
1710+
RouterModule,
1711+
MyDir,
1712+
],
1713+
exports: [MyDir],
1714+
})
1715+
`));
1716+
});
1717+
1718+
it('should preserve the trailing comma when marking a directive as standalone', async () => {
1719+
writeFile('module.ts', `
1720+
import {NgModule, Directive} from '@angular/core';
1721+
1722+
@Directive({
1723+
selector: '[dir]',
1724+
exportAs: 'dir',
1725+
})
1726+
export class MyDir {}
1727+
1728+
@NgModule({declarations: [MyDir]})
1729+
export class Mod {}
1730+
`);
1731+
1732+
await runMigration('convert-to-standalone');
1733+
1734+
expect(stripWhitespace(tree.readContent('module.ts'))).toContain(stripWhitespace(`
1735+
@Directive({
1736+
selector: '[dir]',
1737+
exportAs: 'dir',
1738+
standalone: true,
1739+
})
1740+
`));
1741+
});
1742+
1743+
it('should add a trailing comma when generating an imports array in a component', async () => {
1744+
writeFile('module.ts', `
1745+
import {NgModule, Directive, Component} from '@angular/core';
1746+
1747+
@Directive({selector: '[dir-one]'})
1748+
export class DirOne {}
1749+
1750+
@Directive({selector: '[dir-two]'})
1751+
export class DirTwo {}
1752+
1753+
@Directive({selector: '[dir-three]'})
1754+
export class DirThree {}
1755+
1756+
@Component({
1757+
selector: 'my-comp',
1758+
template: '<div dir-one dir-two dir-three></div>',
1759+
})
1760+
export class MyComp {}
1761+
1762+
@NgModule({declarations: [DirOne, DirTwo, DirThree, MyComp]})
1763+
export class Mod {}
1764+
`);
1765+
1766+
await runMigration('convert-to-standalone');
1767+
1768+
expect(stripWhitespace(tree.readContent('module.ts'))).toContain(stripWhitespace(`
1769+
@Component({
1770+
selector: 'my-comp',
1771+
template: '<div dir-one dir-two dir-three></div>',
1772+
standalone: true,
1773+
imports: [
1774+
DirOne,
1775+
DirTwo,
1776+
DirThree,
1777+
],
1778+
})
1779+
`));
1780+
});
1781+
16601782
it('should remove a module that only has imports and exports', async () => {
16611783
writeFile('app.module.ts', `
16621784
import {NgModule} from '@angular/core';
@@ -2413,6 +2535,43 @@ describe('standalone migration', () => {
24132535
`));
24142536
});
24152537

2538+
it('should preserve the trailing comma when deleting a module', async () => {
2539+
const initialAppModule = `
2540+
import {NgModule, InjectionToken} from '@angular/core';
2541+
import {CommonModule} from '@angular/common';
2542+
import {RouterModule} from '@angular/router';
2543+
2544+
const token = new InjectionToken('token');
2545+
2546+
@NgModule()
2547+
export class ToDelete {}
2548+
2549+
@NgModule({
2550+
imports: [
2551+
CommonModule,
2552+
ToDelete,
2553+
RouterModule,
2554+
],
2555+
providers: [{provide: token, useValue: 123}],
2556+
})
2557+
export class AppModule {}
2558+
`;
2559+
2560+
writeFile('app.module.ts', initialAppModule);
2561+
2562+
await runMigration('prune-ng-modules');
2563+
2564+
expect(stripWhitespace(tree.readContent('app.module.ts'))).toContain(stripWhitespace(`
2565+
@NgModule({
2566+
imports: [
2567+
CommonModule,
2568+
RouterModule,
2569+
],
2570+
providers: [{provide: token, useValue: 123}],
2571+
})
2572+
`));
2573+
});
2574+
24162575
it('should switch a platformBrowser().bootstrapModule call to bootstrapApplication', async () => {
24172576
writeFile('main.ts', `
24182577
import {AppModule} from './app/app.module';

0 commit comments

Comments
 (0)