Skip to content

Commit ef577b2

Browse files
crisbetoalxhub
authored andcommitted
fix(migrations): delete constructor if it only has super call (angular#58013)
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 angular#58013
1 parent 81a5c5d commit ef577b2

File tree

2 files changed

+97
-4
lines changed

2 files changed

+97
-4
lines changed

packages/core/schematics/ng-generate/inject-migration/migration.ts

Lines changed: 28 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -215,10 +215,7 @@ function migrateClass(
215215
}
216216
}
217217

218-
if (
219-
!options.backwardsCompatibleConstructors &&
220-
(!constructor.body || constructor.body.statements.length - removedStatementCount === 0)
221-
) {
218+
if (canRemoveConstructor(options, constructor, removedStatementCount, superCall)) {
222219
// Drop the constructor if it was empty.
223220
removedMembers.add(constructor);
224221
tracker.replaceText(sourceFile, constructor.getFullStart(), constructor.getFullWidth(), '');
@@ -661,3 +658,30 @@ function cloneName(node: ts.PropertyName): ts.PropertyName {
661658
return node;
662659
}
663660
}
661+
662+
/**
663+
* Determines whether it's safe to delete a class constructor.
664+
* @param options Options used to configure the migration.
665+
* @param constructor Node representing the constructor.
666+
* @param removedStatementCount Number of statements that were removed by the migration.
667+
* @param superCall Node representing the `super()` call within the constructor.
668+
*/
669+
function canRemoveConstructor(
670+
options: MigrationOptions,
671+
constructor: ts.ConstructorDeclaration,
672+
removedStatementCount: number,
673+
superCall: ts.CallExpression | null,
674+
): boolean {
675+
if (options.backwardsCompatibleConstructors) {
676+
return false;
677+
}
678+
679+
const statementCount = constructor.body
680+
? constructor.body.statements.length - removedStatementCount
681+
: 0;
682+
683+
return (
684+
statementCount === 0 ||
685+
(statementCount === 1 && superCall !== null && superCall.arguments.length === 0)
686+
);
687+
}

packages/core/schematics/test/inject_migration_spec.ts

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -901,6 +901,37 @@ describe('inject migration', () => {
901901
]);
902902
});
903903

904+
it('should remove the constructor if it only has a super() call after the migration', async () => {
905+
writeFile(
906+
'/dir.ts',
907+
[
908+
`import { Directive } from '@angular/core';`,
909+
`import { Parent } from './parent';`,
910+
`import { SomeService } from './service';`,
911+
``,
912+
`@Directive()`,
913+
`class MyDir extends Parent {`,
914+
` constructor(private service: SomeService) {`,
915+
` super();`,
916+
` }`,
917+
`}`,
918+
].join('\n'),
919+
);
920+
921+
await runMigration();
922+
923+
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
924+
`import { Directive, inject } from '@angular/core';`,
925+
`import { Parent } from './parent';`,
926+
`import { SomeService } from './service';`,
927+
``,
928+
`@Directive()`,
929+
`class MyDir extends Parent {`,
930+
` private service = inject(SomeService);`,
931+
`}`,
932+
]);
933+
});
934+
904935
it('should be able to opt into generating backwards-compatible constructors for a class with existing members', async () => {
905936
writeFile(
906937
'/dir.ts',
@@ -987,6 +1018,44 @@ describe('inject migration', () => {
9871018
]);
9881019
});
9891020

1021+
it('should not remove the constructor, even if it only has a super call, if backwards compatible constructors are enabled', async () => {
1022+
writeFile(
1023+
'/dir.ts',
1024+
[
1025+
`import { Directive } from '@angular/core';`,
1026+
`import { Parent } from './parent';`,
1027+
`import { SomeService } from './service';`,
1028+
``,
1029+
`@Directive()`,
1030+
`class MyDir extends Parent {`,
1031+
` constructor(private service: SomeService) {`,
1032+
` super();`,
1033+
` }`,
1034+
`}`,
1035+
].join('\n'),
1036+
);
1037+
1038+
await runMigration({backwardsCompatibleConstructors: true});
1039+
1040+
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
1041+
`import { Directive, inject } from '@angular/core';`,
1042+
`import { Parent } from './parent';`,
1043+
`import { SomeService } from './service';`,
1044+
``,
1045+
`@Directive()`,
1046+
`class MyDir extends Parent {`,
1047+
` private service = inject(SomeService);`,
1048+
``,
1049+
` /** Inserted by Angular inject() migration for backwards compatibility */`,
1050+
` constructor(...args: unknown[]);`,
1051+
``,
1052+
` constructor() {`,
1053+
` super();`,
1054+
` }`,
1055+
`}`,
1056+
]);
1057+
});
1058+
9901059
it('should not migrate abstract classes by default', async () => {
9911060
const initialContent = [
9921061
`import { Directive } from '@angular/core';`,

0 commit comments

Comments
 (0)