Skip to content

Commit 1710cbd

Browse files
SkyZeroZxkirjs
SkyZeroZx
authored and
kirjs
committed
fix(migrations): handle shorthand property declarations in NgModule (#64160)
The migration now correctly detects shorthand declarations in NgModule metadata PR Close #64160
1 parent 0e928fb commit 1710cbd

File tree

2 files changed

+299
-54
lines changed

2 files changed

+299
-54
lines changed

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

Lines changed: 76 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -314,33 +314,43 @@ function moveDeclarationsToImports(
314314
);
315315

316316
// Separate the declarations that we want to keep and ones we need to copy into the `imports`.
317-
if (ts.isPropertyAssignment(declarationsProp)) {
318-
// If the declarations are an array, we can analyze it to
319-
// find any classes from the current migration.
320-
if (ts.isArrayLiteralExpression(declarationsProp.initializer)) {
321-
for (const el of declarationsProp.initializer.elements) {
322-
if (ts.isIdentifier(el)) {
323-
const correspondingClass = findClassDeclaration(el, typeChecker);
324-
325-
if (
326-
!correspondingClass ||
327-
// Check whether the declaration is either standalone already or is being converted
328-
// in this migration. We need to check if it's standalone already, in order to correct
329-
// some cases where the main app and the test files are being migrated in separate
330-
// programs.
331-
isStandaloneDeclaration(correspondingClass, allDeclarations, templateTypeChecker)
332-
) {
333-
declarationsToCopy.push(el);
317+
if (
318+
ts.isPropertyAssignment(declarationsProp) ||
319+
ts.isShorthandPropertyAssignment(declarationsProp)
320+
) {
321+
// Handle both regular and shorthand property assignments
322+
if (ts.isPropertyAssignment(declarationsProp)) {
323+
// If the declarations are an array, we can analyze it to
324+
// find any classes from the current migration.
325+
if (ts.isArrayLiteralExpression(declarationsProp.initializer)) {
326+
for (const el of declarationsProp.initializer.elements) {
327+
if (ts.isIdentifier(el)) {
328+
const correspondingClass = findClassDeclaration(el, typeChecker);
329+
330+
if (
331+
!correspondingClass ||
332+
// Check whether the declaration is either standalone already or is being converted
333+
// in this migration. We need to check if it's standalone already, in order to correct
334+
// some cases where the main app and the test files are being migrated in separate
335+
// programs.
336+
isStandaloneDeclaration(correspondingClass, allDeclarations, templateTypeChecker)
337+
) {
338+
declarationsToCopy.push(el);
339+
} else {
340+
declarationsToPreserve.push(el);
341+
}
334342
} else {
335-
declarationsToPreserve.push(el);
343+
declarationsToCopy.push(el);
336344
}
337-
} else {
338-
declarationsToCopy.push(el);
339345
}
346+
} else {
347+
// Otherwise create a spread that will be copied into the `imports`.
348+
declarationsToCopy.push(ts.factory.createSpreadElement(declarationsProp.initializer));
340349
}
341350
} else {
342-
// Otherwise create a spread that will be copied into the `imports`.
343-
declarationsToCopy.push(ts.factory.createSpreadElement(declarationsProp.initializer));
351+
// For shorthand properties, treat them as unanalyzable and use spread syntax
352+
// shorthand properties were being ignored, now they're detected and treated as spreads
353+
declarationsToCopy.push(ts.factory.createSpreadElement(declarationsProp.name));
344354
}
345355
}
346356

@@ -360,20 +370,21 @@ function moveDeclarationsToImports(
360370
}
361371

362372
for (const prop of literal.properties) {
363-
if (!isNamedPropertyAssignment(prop)) {
373+
if (!isNamedPropertyAssignment(prop) && !ts.isShorthandPropertyAssignment(prop)) {
364374
properties.push(prop);
365375
continue;
366376
}
367377

368378
// If we have declarations to preserve, update the existing property, otherwise drop it.
369379
if (prop === declarationsProp) {
370380
if (declarationsToPreserve.length > 0) {
371-
const hasTrailingComma = ts.isArrayLiteralExpression(prop.initializer)
372-
? prop.initializer.elements.hasTrailingComma
373-
: hasAnyArrayTrailingComma;
381+
const hasTrailingComma =
382+
ts.isPropertyAssignment(prop) && ts.isArrayLiteralExpression(prop.initializer)
383+
? prop.initializer.elements.hasTrailingComma
384+
: hasAnyArrayTrailingComma;
385+
374386
properties.push(
375-
ts.factory.updatePropertyAssignment(
376-
prop,
387+
ts.factory.createPropertyAssignment(
377388
prop.name,
378389
ts.factory.createArrayLiteralExpression(
379390
ts.factory.createNodeArray(
@@ -390,29 +401,32 @@ function moveDeclarationsToImports(
390401
// If we have an `imports` array and declarations
391402
// that should be copied, we merge the two arrays.
392403
if (prop === importsProp && declarationsToCopy.length > 0) {
393-
let initializer: ts.Expression;
404+
// Only regular property assignments have initializers that we can merge
405+
if (ts.isPropertyAssignment(prop)) {
406+
let initializer: ts.Expression;
407+
408+
if (ts.isArrayLiteralExpression(prop.initializer)) {
409+
initializer = ts.factory.updateArrayLiteralExpression(
410+
prop.initializer,
411+
ts.factory.createNodeArray(
412+
[...prop.initializer.elements, ...declarationsToCopy],
413+
prop.initializer.elements.hasTrailingComma,
414+
),
415+
);
416+
} else {
417+
initializer = ts.factory.createArrayLiteralExpression(
418+
ts.factory.createNodeArray(
419+
[ts.factory.createSpreadElement(prop.initializer), ...declarationsToCopy],
420+
// Expect the declarations to be greater than 1 since
421+
// we have the pre-existing initializer already.
422+
hasAnyArrayTrailingComma && declarationsToCopy.length > 1,
423+
),
424+
);
425+
}
394426

395-
if (ts.isArrayLiteralExpression(prop.initializer)) {
396-
initializer = ts.factory.updateArrayLiteralExpression(
397-
prop.initializer,
398-
ts.factory.createNodeArray(
399-
[...prop.initializer.elements, ...declarationsToCopy],
400-
prop.initializer.elements.hasTrailingComma,
401-
),
402-
);
403-
} else {
404-
initializer = ts.factory.createArrayLiteralExpression(
405-
ts.factory.createNodeArray(
406-
[ts.factory.createSpreadElement(prop.initializer), ...declarationsToCopy],
407-
// Expect the declarations to be greater than 1 since
408-
// we have the pre-existing initializer already.
409-
hasAnyArrayTrailingComma && declarationsToCopy.length > 1,
410-
),
411-
);
427+
properties.push(ts.factory.updatePropertyAssignment(prop, prop.name, initializer));
428+
continue;
412429
}
413-
414-
properties.push(ts.factory.updatePropertyAssignment(prop, prop.name, initializer));
415-
continue;
416430
}
417431

418432
// Retain any remaining properties.
@@ -566,11 +580,17 @@ export function findImportLocation(
566580
* E.g. `declarations: [Foo]` or `declarations: SOME_VAR` would match this description,
567581
* but not `declarations: []`.
568582
*/
569-
function hasNgModuleMetadataElements(node: ts.Node): node is ts.PropertyAssignment {
570-
return (
571-
ts.isPropertyAssignment(node) &&
572-
(!ts.isArrayLiteralExpression(node.initializer) || node.initializer.elements.length > 0)
573-
);
583+
function hasNgModuleMetadataElements(
584+
node: ts.Node,
585+
): node is ts.PropertyAssignment | ts.ShorthandPropertyAssignment {
586+
if (ts.isPropertyAssignment(node)) {
587+
return !ts.isArrayLiteralExpression(node.initializer) || node.initializer.elements.length > 0;
588+
}
589+
if (ts.isShorthandPropertyAssignment(node)) {
590+
// For shorthand properties, we assume they have elements since they reference a variable
591+
return true;
592+
}
593+
return false;
574594
}
575595

576596
/** Finds all modules whose declarations can be migrated. */
@@ -817,6 +837,7 @@ function analyzeTestingModules(
817837
const importElements =
818838
importsProp &&
819839
hasNgModuleMetadataElements(importsProp) &&
840+
ts.isPropertyAssignment(importsProp) &&
820841
ts.isArrayLiteralExpression(importsProp.initializer)
821842
? importsProp.initializer.elements.filter((el) => {
822843
// Filter out calls since they may be a `ModuleWithProviders`.
@@ -879,6 +900,7 @@ function extractDeclarationsFromTestObject(
879900
if (
880901
declarations &&
881902
hasNgModuleMetadataElements(declarations) &&
903+
ts.isPropertyAssignment(declarations) &&
882904
ts.isArrayLiteralExpression(declarations.initializer)
883905
) {
884906
for (const element of declarations.initializer.elements) {

0 commit comments

Comments
 (0)