fix(migrations): handle shorthand property declarations in NgModule (#64160)

The migration now correctly detects shorthand declarations in NgModule metadata PR Close #64160
2026-09-14 13:54:52 +08:00 · 2025-09-30 13:39:26 -05:00
parent 5319177638
commit 51a0b59389
2 changed files with 298 additions and 53 deletions
@@ -314,33 +314,43 @@ function moveDeclarationsToImports(
  );

  // Separate the declarations that we want to keep and ones we need to copy into the `imports`.
-  if (ts.isPropertyAssignment(declarationsProp)) {
-    // If the declarations are an array, we can analyze it to
-    // find any classes from the current migration.
-    if (ts.isArrayLiteralExpression(declarationsProp.initializer)) {
-      for (const el of declarationsProp.initializer.elements) {
-        if (ts.isIdentifier(el)) {
-          const correspondingClass = findClassDeclaration(el, typeChecker);
+  if (
+    ts.isPropertyAssignment(declarationsProp) ||
+    ts.isShorthandPropertyAssignment(declarationsProp)
+  ) {
+    // Handle both regular and shorthand property assignments
+    if (ts.isPropertyAssignment(declarationsProp)) {
+      // If the declarations are an array, we can analyze it to
+      // find any classes from the current migration.
+      if (ts.isArrayLiteralExpression(declarationsProp.initializer)) {
+        for (const el of declarationsProp.initializer.elements) {
+          if (ts.isIdentifier(el)) {
+            const correspondingClass = findClassDeclaration(el, typeChecker);

-          if (
-            !correspondingClass ||
-            // Check whether the declaration is either standalone already or is being converted
-            // in this migration. We need to check if it's standalone already, in order to correct
-            // some cases where the main app and the test files are being migrated in separate
-            // programs.
-            isStandaloneDeclaration(correspondingClass, allDeclarations, templateTypeChecker)
-          ) {
-            declarationsToCopy.push(el);
+            if (
+              !correspondingClass ||
+              // Check whether the declaration is either standalone already or is being converted
+              // in this migration. We need to check if it's standalone already, in order to correct
+              // some cases where the main app and the test files are being migrated in separate
+              // programs.
+              isStandaloneDeclaration(correspondingClass, allDeclarations, templateTypeChecker)
+            ) {
+              declarationsToCopy.push(el);
+            } else {
+              declarationsToPreserve.push(el);
+            }
          } else {
-            declarationsToPreserve.push(el);
+            declarationsToCopy.push(el);
          }
-        } else {
-          declarationsToCopy.push(el);
        }
+      } else {
+        // Otherwise create a spread that will be copied into the `imports`.
+        declarationsToCopy.push(ts.factory.createSpreadElement(declarationsProp.initializer));
      }
    } else {
-      // Otherwise create a spread that will be copied into the `imports`.
-      declarationsToCopy.push(ts.factory.createSpreadElement(declarationsProp.initializer));
+      // For shorthand properties, treat them as unanalyzable and use spread syntax
+      // shorthand properties were being ignored, now they're detected and treated as spreads
+      declarationsToCopy.push(ts.factory.createSpreadElement(declarationsProp.name));
    }
  }

@@ -360,7 +370,7 @@ function moveDeclarationsToImports(
  }

  for (const prop of literal.properties) {
-    if (!isNamedPropertyAssignment(prop)) {
+    if (!isNamedPropertyAssignment(prop) && !ts.isShorthandPropertyAssignment(prop)) {
      properties.push(prop);
      continue;
    }
@@ -368,12 +378,13 @@ function moveDeclarationsToImports(
    // If we have declarations to preserve, update the existing property, otherwise drop it.
    if (prop === declarationsProp) {
      if (declarationsToPreserve.length > 0) {
-        const hasTrailingComma = ts.isArrayLiteralExpression(prop.initializer)
-          ? prop.initializer.elements.hasTrailingComma
-          : hasAnyArrayTrailingComma;
+        const hasTrailingComma =
+          ts.isPropertyAssignment(prop) && ts.isArrayLiteralExpression(prop.initializer)
+            ? prop.initializer.elements.hasTrailingComma
+            : hasAnyArrayTrailingComma;
+
        properties.push(
-          ts.factory.updatePropertyAssignment(
-            prop,
+          ts.factory.createPropertyAssignment(
            prop.name,
            ts.factory.createArrayLiteralExpression(
              ts.factory.createNodeArray(
@@ -390,29 +401,32 @@ function moveDeclarationsToImports(
    // If we have an `imports` array and declarations
    // that should be copied, we merge the two arrays.
    if (prop === importsProp && declarationsToCopy.length > 0) {
-      let initializer: ts.Expression;
+      // Only regular property assignments have initializers that we can merge
+      if (ts.isPropertyAssignment(prop)) {
+        let initializer: ts.Expression;

-      if (ts.isArrayLiteralExpression(prop.initializer)) {
-        initializer = ts.factory.updateArrayLiteralExpression(
-          prop.initializer,
-          ts.factory.createNodeArray(
-            [...prop.initializer.elements, ...declarationsToCopy],
-            prop.initializer.elements.hasTrailingComma,
-          ),
-        );
-      } else {
-        initializer = ts.factory.createArrayLiteralExpression(
-          ts.factory.createNodeArray(
-            [ts.factory.createSpreadElement(prop.initializer), ...declarationsToCopy],
-            // Expect the declarations to be greater than 1 since
-            // we have the pre-existing initializer already.
-            hasAnyArrayTrailingComma && declarationsToCopy.length > 1,
-          ),
-        );
+        if (ts.isArrayLiteralExpression(prop.initializer)) {
+          initializer = ts.factory.updateArrayLiteralExpression(
+            prop.initializer,
+            ts.factory.createNodeArray(
+              [...prop.initializer.elements, ...declarationsToCopy],
+              prop.initializer.elements.hasTrailingComma,
+            ),
+          );
+        } else {
+          initializer = ts.factory.createArrayLiteralExpression(
+            ts.factory.createNodeArray(
+              [ts.factory.createSpreadElement(prop.initializer), ...declarationsToCopy],
+              // Expect the declarations to be greater than 1 since
+              // we have the pre-existing initializer already.
+              hasAnyArrayTrailingComma && declarationsToCopy.length > 1,
+            ),
+          );
+        }
+
+        properties.push(ts.factory.updatePropertyAssignment(prop, prop.name, initializer));
+        continue;
      }
-
-      properties.push(ts.factory.updatePropertyAssignment(prop, prop.name, initializer));
-      continue;
    }

    // Retain any remaining properties.
@@ -566,11 +580,17 @@ export function findImportLocation(
 * E.g. `declarations: [Foo]` or `declarations: SOME_VAR` would match this description,
 * but not `declarations: []`.
 */
-function hasNgModuleMetadataElements(node: ts.Node): node is ts.PropertyAssignment {
-  return (
-    ts.isPropertyAssignment(node) &&
-    (!ts.isArrayLiteralExpression(node.initializer) || node.initializer.elements.length > 0)
-  );
+function hasNgModuleMetadataElements(
+  node: ts.Node,
+): node is ts.PropertyAssignment | ts.ShorthandPropertyAssignment {
+  if (ts.isPropertyAssignment(node)) {
+    return !ts.isArrayLiteralExpression(node.initializer) || node.initializer.elements.length > 0;
+  }
+  if (ts.isShorthandPropertyAssignment(node)) {
+    // For shorthand properties, we assume they have elements since they reference a variable
+    return true;
+  }
+  return false;
 }

 /** Finds all modules whose declarations can be migrated. */
@@ -817,6 +837,7 @@ function analyzeTestingModules(
    const importElements =
      importsProp &&
      hasNgModuleMetadataElements(importsProp) &&
+      ts.isPropertyAssignment(importsProp) &&
      ts.isArrayLiteralExpression(importsProp.initializer)
        ? importsProp.initializer.elements.filter((el) => {
            // Filter out calls since they may be a `ModuleWithProviders`.
@@ -879,6 +900,7 @@ function extractDeclarationsFromTestObject(
  if (
    declarations &&
    hasNgModuleMetadataElements(declarations) &&
+    ts.isPropertyAssignment(declarations) &&
    ts.isArrayLiteralExpression(declarations.initializer)
  ) {
    for (const element of declarations.initializer.elements) {
@@ -5123,4 +5123,227 @@ describe('standalone migration', () => {
    `),
    );
  });
+
+  it('should handle shorthand property assignment in module declarations', async () => {
+    writeFile(
+      'module.ts',
+      `
+      import {NgModule, Directive, Component} from '@angular/core';
+
+      @Directive({selector: '[dir]', standalone: false})
+      export class BuzzComponent {}
+
+      @Component({selector: 'fizz', template: 'fizz', standalone: false})
+      export class FizzComponent {}
+
+      @Component({selector: 'bar', template: 'bar', standalone: true})
+      export class BarComponent {}
+
+      @Component({selector: 'foo', template: 'foo', standalone: true})
+      export class FooComponent {}
+
+      const declarations = [BuzzComponent, FizzComponent];
+
+      @NgModule({
+        declarations,
+        imports: [BarComponent, FooComponent],
+        exports: [BarComponent, BuzzComponent, FizzComponent, FooComponent],
+      })
+      export class SharedModule {}
+    `,
+    );
+
+    await runMigration('convert-to-standalone');
+
+    const result = tree.readContent('module.ts');
+
+    expect(stripWhitespace(result)).toContain(stripWhitespace(`@Directive({selector: '[dir]'})`));
+    expect(stripWhitespace(result)).toContain(
+      stripWhitespace(`@Component({selector: 'fizz', template: 'fizz'})`),
+    );
+
+    expect(result).not.toContain('standalone: false');
+
+    expect(stripWhitespace(result)).toContain(
+      stripWhitespace(`@NgModule({
+        imports: [BarComponent, FooComponent, ...declarations],
+        exports: [BarComponent, BuzzComponent, FizzComponent, FooComponent],
+      })`),
+    );
+
+    expect(result).toContain('const declarations = [BuzzComponent, FizzComponent];');
+
+    expect(result).toContain('exports: [BarComponent, BuzzComponent, FizzComponent, FooComponent]');
+  });
+
+  it('should handle shorthand property assignment with mixed imports and declarations', async () => {
+    writeFile(
+      'module.ts',
+      `
+      import {NgModule, Component} from '@angular/core';
+      import {NgIf} from '@angular/common';
+
+      @Component({selector: 'my-comp', template: '<div *ngIf="show">Content</div>', standalone: false})
+      export class MyComponent {}
+
+      @Component({selector: 'other-comp', template: 'other', standalone: false})
+      export class OtherComponent {}
+
+      const declarations = [MyComponent, OtherComponent];
+      const imports = [NgIf];
+
+      @NgModule({
+        declarations,
+        imports,
+        exports: [MyComponent, OtherComponent],
+      })
+      export class TestModule {}
+    `,
+    );
+
+    await runMigration('convert-to-standalone');
+
+    const result = tree.readContent('module.ts');
+
+    expect(result).toContain(`import {NgIf} from '@angular/common'`);
+    expect(stripWhitespace(result)).toContain(
+      stripWhitespace(`@Component({
+        selector: 'my-comp',
+        template: '<div *ngIf="show">Content</div>',
+        imports: [NgIf]
+      })`),
+    );
+    expect(stripWhitespace(result)).toContain(
+      stripWhitespace(`@Component({selector: 'other-comp', template: 'other'})`),
+    );
+
+    expect(result).not.toContain('standalone: false');
+
+    expect(stripWhitespace(result)).toContain(
+      stripWhitespace(`@NgModule({
+        imports,
+        exports: [MyComponent, OtherComponent],
+      })`),
+    );
+
+    expect(result).toContain('const declarations = [MyComponent, OtherComponent];');
+    expect(result).toContain('const imports = [NgIf];');
+  });
+
+  it('should handle shorthand property assignment with spread elements in declarations', async () => {
+    writeFile(
+      'module.ts',
+      `
+      import {NgModule, Component} from '@angular/core';
+
+      @Component({selector: 'comp1', template: 'comp1', standalone: false})
+      export class Component1 {}
+
+      @Component({selector: 'comp2', template: 'comp2', standalone: false})  
+      export class Component2 {}
+
+      const staticDeclarationsA = [Component1];
+      const staticDeclarationsB = [Component2];
+      const declarations = [...staticDeclarationsA, ...staticDeclarationsB];
+
+      @NgModule({
+        declarations,
+        exports: [Component1, Component2],
+      })
+      export class MixedModule {}
+    `,
+    );
+
+    await runMigration('convert-to-standalone');
+
+    const result = tree.readContent('module.ts');
+
+    expect(stripWhitespace(result)).toContain(
+      stripWhitespace(`@Component({selector: 'comp1', template: 'comp1'})`),
+    );
+    expect(stripWhitespace(result)).toContain(
+      stripWhitespace(`@Component({selector: 'comp2', template: 'comp2'})`),
+    );
+    expect(result).not.toContain('standalone: false');
+
+    expect(stripWhitespace(result)).toContain(
+      stripWhitespace(`@NgModule({
+        imports: [...declarations],
+        exports: [Component1, Component2],
+      })`),
+    );
+
+    expect(result).toContain('const staticDeclarationsA = [Component1];');
+    expect(result).toContain('const staticDeclarationsB = [Component2];');
+    expect(result).toContain(
+      'const declarations = [...staticDeclarationsA, ...staticDeclarationsB];',
+    );
+  });
+
+  it('should handle both regular and shorthand property assignments in the same migration', async () => {
+    writeFile(
+      'regular-module.ts',
+      `
+      import {NgModule, Component} from '@angular/core';
+
+      @Component({selector: 'regular-comp', template: 'regular', standalone: false})
+      export class RegularComponent {}
+
+      @NgModule({
+        declarations: [RegularComponent],
+        exports: [RegularComponent],
+      })
+      export class RegularModule {}
+    `,
+    );
+
+    writeFile(
+      'shorthand-module.ts',
+      `
+      import {NgModule, Component} from '@angular/core';
+
+      @Component({selector: 'shorthand-comp', template: 'shorthand', standalone: false})
+      export class ShorthandComponent {}
+
+      const declarations = [ShorthandComponent];
+
+      @NgModule({
+        declarations,
+        exports: [ShorthandComponent],
+      })
+      export class ShorthandModule {}
+    `,
+    );
+
+    await runMigration('convert-to-standalone');
+
+    const regularResult = tree.readContent('regular-module.ts');
+    const shorthandResult = tree.readContent('shorthand-module.ts');
+
+    expect(stripWhitespace(regularResult)).toContain(
+      stripWhitespace(`@Component({selector: 'regular-comp', template: 'regular'})`),
+    );
+    expect(stripWhitespace(regularResult)).toContain(
+      stripWhitespace(`@NgModule({
+        imports: [RegularComponent],
+        exports: [RegularComponent],
+      })`),
+    );
+
+    expect(stripWhitespace(shorthandResult)).toContain(
+      stripWhitespace(`@Component({selector: 'shorthand-comp', template: 'shorthand'})`),
+    );
+
+    expect(stripWhitespace(shorthandResult)).toContain(
+      stripWhitespace(`@NgModule({
+        imports: [...declarations],
+        exports: [ShorthandComponent],
+      })`),
+    );
+
+    expect(shorthandResult).toContain('const declarations = [ShorthandComponent];');
+
+    expect(regularResult).not.toContain('standalone: false');
+    expect(shorthandResult).not.toContain('standalone: false');
+  });
 });