fix(migrations): delete constructor if it only has super call (#58013)

crisbeto · alxhub · commit b9d846dad778 · 2024-09-30T13:39:43.000-07:00
Adds some logic to the `inject` migration that will remove constructors that are made up of only a `super` call after the migration. PR Close #58013
diff --git a/packages/core/schematics/ng-generate/inject-migration/migration.ts b/packages/core/schematics/ng-generate/inject-migration/migration.ts
@@ -215,10 +215,7 @@ function migrateClass(
     }
   }
 
-  if (
-    !options.backwardsCompatibleConstructors &&
-    (!constructor.body || constructor.body.statements.length - removedStatementCount === 0)
-  ) {
+  if (canRemoveConstructor(options, constructor, removedStatementCount, superCall)) {
     // Drop the constructor if it was empty.
     removedMembers.add(constructor);
     tracker.replaceText(sourceFile, constructor.getFullStart(), constructor.getFullWidth(), '');
@@ -661,3 +658,30 @@ function cloneName(node: ts.PropertyName): ts.PropertyName {
       return node;
   }
 }
+
+/**
+ * Determines whether it's safe to delete a class constructor.
+ * @param options Options used to configure the migration.
+ * @param constructor Node representing the constructor.
+ * @param removedStatementCount Number of statements that were removed by the migration.
+ * @param superCall Node representing the `super()` call within the constructor.
+ */
+function canRemoveConstructor(
+  options: MigrationOptions,
+  constructor: ts.ConstructorDeclaration,
+  removedStatementCount: number,
+  superCall: ts.CallExpression | null,
+): boolean {
+  if (options.backwardsCompatibleConstructors) {
+    return false;
+  }
+
+  const statementCount = constructor.body
+    ? constructor.body.statements.length - removedStatementCount
+    : 0;
+
+  return (
+    statementCount === 0 ||
+    (statementCount === 1 && superCall !== null && superCall.arguments.length === 0)
+  );
+}
diff --git a/packages/core/schematics/test/inject_migration_spec.ts b/packages/core/schematics/test/inject_migration_spec.ts
@@ -901,6 +901,37 @@ describe('inject migration', () => {
     ]);
   });
 
+  it('should remove the constructor if it only has a super() call after the migration', async () => {
+    writeFile(
+      '/dir.ts',
+      [
+        `import { Directive } from '@angular/core';`,
+        `import { Parent } from './parent';`,
+        `import { SomeService } from './service';`,
+        ``,
+        `@Directive()`,
+        `class MyDir extends Parent {`,
+        `  constructor(private service: SomeService) {`,
+        `    super();`,
+        `  }`,
+        `}`,
+      ].join('\n'),
+    );
+
+    await runMigration();
+
+    expect(tree.readContent('/dir.ts').split('\n')).toEqual([
+      `import { Directive, inject } from '@angular/core';`,
+      `import { Parent } from './parent';`,
+      `import { SomeService } from './service';`,
+      ``,
+      `@Directive()`,
+      `class MyDir extends Parent {`,
+      `  private service = inject(SomeService);`,
+      `}`,
+    ]);
+  });
+
   it('should be able to opt into generating backwards-compatible constructors for a class with existing members', async () => {
     writeFile(
       '/dir.ts',
@@ -987,6 +1018,44 @@ describe('inject migration', () => {
     ]);
   });
 
+  it('should not remove the constructor, even if it only has a super call, if backwards compatible constructors are enabled', async () => {
+    writeFile(
+      '/dir.ts',
+      [
+        `import { Directive } from '@angular/core';`,
+        `import { Parent } from './parent';`,
+        `import { SomeService } from './service';`,
+        ``,
+        `@Directive()`,
+        `class MyDir extends Parent {`,
+        `  constructor(private service: SomeService) {`,
+        `    super();`,
+        `  }`,
+        `}`,
+      ].join('\n'),
+    );
+
+    await runMigration({backwardsCompatibleConstructors: true});
+
+    expect(tree.readContent('/dir.ts').split('\n')).toEqual([
+      `import { Directive, inject } from '@angular/core';`,
+      `import { Parent } from './parent';`,
+      `import { SomeService } from './service';`,
+      ``,
+      `@Directive()`,
+      `class MyDir extends Parent {`,
+      `  private service = inject(SomeService);`,
+      ``,
+      `  /** Inserted by Angular inject() migration for backwards compatibility */`,
+      `  constructor(...args: unknown[]);`,
+      ``,
+      `  constructor() {`,
+      `    super();`,
+      `  }`,
+      `}`,
+    ]);
+  });
+
   it('should not migrate abstract classes by default', async () => {
     const initialContent = [
       `import { Directive } from '@angular/core';`,
