Skip to content

Commit d4f5c85

Browse files
crisbetoAndrewKushnir
crisbeto
authored and
AndrewKushnir
committed
fix(migrations): handle parameters with initializers in inject migration (#58769)
Adds some logic to the `inject()` migration that allows is to handle parameter declarations with initializers. This was omitted initially, because we don't officially support such cases, but it came up internally. PR Close #58769
1 parent baedea7 commit d4f5c85

File tree

3 files changed

+258
-13
lines changed

3 files changed

+258
-13
lines changed

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

Lines changed: 55 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -185,14 +185,14 @@ export function getConstructorUnusedParameters(
185185
return topLevelParameters;
186186
}
187187

188-
declaration.body.forEachChild(function walk(node) {
188+
const analyze = (node: ts.Node) => {
189189
// Don't descend into statements that were removed already.
190190
if (ts.isStatement(node) && removedStatements.has(node)) {
191191
return;
192192
}
193193

194194
if (!ts.isIdentifier(node) || !topLevelParameterNames.has(node.text)) {
195-
node.forEachChild(walk);
195+
node.forEachChild(analyze);
196196
return;
197197
}
198198

@@ -213,8 +213,16 @@ export function getConstructorUnusedParameters(
213213
}
214214
}
215215
});
216+
};
217+
218+
declaration.parameters.forEach((param) => {
219+
if (param.initializer) {
220+
analyze(param.initializer);
221+
}
216222
});
217223

224+
declaration.body.forEachChild(analyze);
225+
218226
for (const param of topLevelParameters) {
219227
if (!accessedTopLevelParameters.has(param)) {
220228
unusedParams.add(param);
@@ -261,6 +269,51 @@ export function getSuperParameters(
261269
return usedParams;
262270
}
263271

272+
/**
273+
* Determines if a specific parameter has references to other parameters.
274+
* @param param Parameter to check.
275+
* @param allParameters All parameters of the containing function.
276+
* @param localTypeChecker Type checker scoped to the current file.
277+
*/
278+
export function parameterReferencesOtherParameters(
279+
param: ts.ParameterDeclaration,
280+
allParameters: ts.NodeArray<ts.ParameterDeclaration>,
281+
localTypeChecker: ts.TypeChecker,
282+
): boolean {
283+
// A parameter can only reference other parameters through its initializer.
284+
if (!param.initializer || allParameters.length < 2) {
285+
return false;
286+
}
287+
288+
const paramNames = new Set<string>();
289+
for (const current of allParameters) {
290+
if (current !== param && ts.isIdentifier(current.name)) {
291+
paramNames.add(current.name.text);
292+
}
293+
}
294+
295+
let result = false;
296+
const analyze = (node: ts.Node) => {
297+
if (ts.isIdentifier(node) && paramNames.has(node.text) && !isAccessedViaThis(node)) {
298+
const symbol = localTypeChecker.getSymbolAtLocation(node);
299+
const referencesOtherParam = symbol?.declarations?.some((decl) => {
300+
return (allParameters as ts.NodeArray<ts.Declaration>).includes(decl);
301+
});
302+
303+
if (referencesOtherParam) {
304+
result = true;
305+
}
306+
}
307+
308+
if (!result) {
309+
node.forEachChild(analyze);
310+
}
311+
};
312+
313+
analyze(param.initializer);
314+
return result;
315+
}
316+
264317
/** Checks whether a parameter node declares a property on its class. */
265318
export function parameterDeclaresProperty(node: ts.ParameterDeclaration): boolean {
266319
return !!node.modifiers?.some(

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

Lines changed: 60 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import {
1717
parameterDeclaresProperty,
1818
DI_PARAM_SYMBOLS,
1919
MigrationOptions,
20+
parameterReferencesOtherParameters,
2021
} from './analysis';
2122
import {getAngularDecorators} from '../../utils/ng_decorators';
2223
import {getImportOfIdentifier} from '../../utils/typescript/imports';
@@ -149,6 +150,11 @@ function migrateClass(
149150
for (const param of constructor.parameters) {
150151
const usedInSuper = superParameters !== null && superParameters.has(param);
151152
const usedInConstructor = !unusedParameters.has(param);
153+
const usesOtherParams = parameterReferencesOtherParameters(
154+
param,
155+
constructor.parameters,
156+
localTypeChecker,
157+
);
152158

153159
migrateParameter(
154160
param,
@@ -159,6 +165,7 @@ function migrateClass(
159165
superCall,
160166
usedInSuper,
161167
usedInConstructor,
168+
usesOtherParams,
162169
memberIndentation,
163170
innerIndentation,
164171
prependToConstructor,
@@ -176,7 +183,15 @@ function migrateClass(
176183
}
177184
}
178185

179-
if (canRemoveConstructor(options, constructor, removedStatementCount, superCall)) {
186+
if (
187+
canRemoveConstructor(
188+
options,
189+
constructor,
190+
removedStatementCount,
191+
prependToConstructor,
192+
superCall,
193+
)
194+
) {
180195
// Drop the constructor if it was empty.
181196
removedMembers.add(constructor);
182197
tracker.removeNode(constructor, true);
@@ -186,11 +201,24 @@ function migrateClass(
186201
stripConstructorParameters(constructor, tracker);
187202

188203
if (prependToConstructor.length > 0) {
189-
tracker.insertText(
190-
sourceFile,
191-
(firstConstructorStatement || innerReference).getFullStart(),
192-
`\n${prependToConstructor.join('\n')}\n`,
193-
);
204+
if (
205+
firstConstructorStatement ||
206+
(innerReference !== constructor &&
207+
innerReference.getStart() >= constructor.getStart() &&
208+
innerReference.getEnd() <= constructor.getEnd())
209+
) {
210+
tracker.insertText(
211+
sourceFile,
212+
(firstConstructorStatement || innerReference).getFullStart(),
213+
`\n${prependToConstructor.join('\n')}\n`,
214+
);
215+
} else {
216+
tracker.insertText(
217+
sourceFile,
218+
constructor.body!.getStart() + 1,
219+
`\n${prependToConstructor.map((p) => innerIndentation + p).join('\n')}\n${innerIndentation}`,
220+
);
221+
}
194222
}
195223
}
196224

@@ -268,6 +296,7 @@ function migrateParameter(
268296
superCall: ts.CallExpression | null,
269297
usedInSuper: boolean,
270298
usedInConstructor: boolean,
299+
usesOtherParams: boolean,
271300
memberIndentation: string,
272301
innerIndentation: string,
273302
prependToConstructor: string[],
@@ -290,6 +319,9 @@ function migrateParameter(
290319

291320
// If the parameter declares a property, we need to declare it (e.g. `private foo: Foo`).
292321
if (declaresProp) {
322+
// We can't initialize the property if it's referenced within a `super` call or it references
323+
// other parameters. See the logic further below for the initialization.
324+
const canInitialize = !usedInSuper && !usesOtherParams;
293325
const prop = ts.factory.createPropertyDeclaration(
294326
cloneModifiers(
295327
node.modifiers?.filter((modifier) => {
@@ -302,10 +334,8 @@ function migrateParameter(
302334
node.modifiers?.some((modifier) => modifier.kind === ts.SyntaxKind.PrivateKeyword)
303335
? undefined
304336
: node.questionToken,
305-
// We can't initialize the property if it's referenced within a `super` call.
306-
// See the logic further below for the initialization.
307-
usedInSuper ? node.type : undefined,
308-
usedInSuper ? undefined : ts.factory.createIdentifier(PLACEHOLDER),
337+
canInitialize ? undefined : node.type,
338+
canInitialize ? ts.factory.createIdentifier(PLACEHOLDER) : undefined,
309339
);
310340

311341
propsToAdd.push(
@@ -341,6 +371,14 @@ function migrateParameter(
341371
// don't need to declare any new properties.
342372
prependToConstructor.push(`${innerIndentation}const ${name} = ${replacementCall};`);
343373
}
374+
} else if (usesOtherParams && declaresProp) {
375+
const toAdd = `${innerIndentation}this.${name} = ${replacementCall};`;
376+
377+
if (superCall === null) {
378+
prependToConstructor.push(toAdd);
379+
} else {
380+
afterSuper.push(toAdd);
381+
}
344382
}
345383
}
346384

@@ -449,6 +487,15 @@ function createInjectReplacementCall(
449487
}
450488
}
451489

490+
// If the parameter is initialized, add the initializer as a fallback.
491+
if (param.initializer) {
492+
expression = ts.factory.createBinaryExpression(
493+
expression,
494+
ts.SyntaxKind.QuestionQuestionToken,
495+
param.initializer,
496+
);
497+
}
498+
452499
return replaceNodePlaceholder(param.getSourceFile(), expression, injectedType, printer);
453500
}
454501

@@ -638,15 +685,17 @@ function cloneName(node: ts.PropertyName): ts.PropertyName {
638685
* @param options Options used to configure the migration.
639686
* @param constructor Node representing the constructor.
640687
* @param removedStatementCount Number of statements that were removed by the migration.
688+
* @param prependToConstructor Statements that should be prepended to the constructor.
641689
* @param superCall Node representing the `super()` call within the constructor.
642690
*/
643691
function canRemoveConstructor(
644692
options: MigrationOptions,
645693
constructor: ts.ConstructorDeclaration,
646694
removedStatementCount: number,
695+
prependToConstructor: string[],
647696
superCall: ts.CallExpression | null,
648697
): boolean {
649-
if (options.backwardsCompatibleConstructors) {
698+
if (options.backwardsCompatibleConstructors || prependToConstructor.length > 0) {
650699
return false;
651700
}
652701

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

Lines changed: 143 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1501,6 +1501,149 @@ describe('inject migration', () => {
15011501
]);
15021502
});
15031503

1504+
it('should preserve initializers', async () => {
1505+
writeFile(
1506+
'/dir.ts',
1507+
[
1508+
`import { Directive, Optional } from '@angular/core';`,
1509+
`import { Foo } from './foo';`,
1510+
``,
1511+
`function createFoo() { return new Foo(); }`,
1512+
``,
1513+
`@Directive()`,
1514+
`class MyDir {`,
1515+
` constructor(@Optional() private foo: Foo = createFoo()) {}`,
1516+
`}`,
1517+
].join('\n'),
1518+
);
1519+
1520+
await runMigration();
1521+
1522+
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
1523+
`import { Directive, inject } from '@angular/core';`,
1524+
`import { Foo } from './foo';`,
1525+
``,
1526+
`function createFoo() { return new Foo(); }`,
1527+
``,
1528+
`@Directive()`,
1529+
`class MyDir {`,
1530+
` private foo = inject(Foo, { optional: true }) ?? createFoo();`,
1531+
`}`,
1532+
]);
1533+
});
1534+
1535+
it('should handle initializers referencing other parameters', async () => {
1536+
writeFile(
1537+
'/dir.ts',
1538+
[
1539+
`import { Directive, Optional } from '@angular/core';`,
1540+
`import { Foo, Bar } from './providers';`,
1541+
``,
1542+
`function createFoo(bar: Bar) { return new Foo(bar); }`,
1543+
``,
1544+
`@Directive()`,
1545+
`class MyDir {`,
1546+
` constructor(bar: Bar, @Optional() private foo: Foo = createFoo(bar)) {}`,
1547+
`}`,
1548+
].join('\n'),
1549+
);
1550+
1551+
await runMigration();
1552+
1553+
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
1554+
`import { Directive, inject } from '@angular/core';`,
1555+
`import { Foo, Bar } from './providers';`,
1556+
``,
1557+
`function createFoo(bar: Bar) { return new Foo(bar); }`,
1558+
``,
1559+
`@Directive()`,
1560+
`class MyDir {`,
1561+
` private foo: Foo;`,
1562+
``,
1563+
` constructor() {`,
1564+
` const bar = inject(Bar);`,
1565+
` this.foo = inject(Foo, { optional: true }) ?? createFoo(bar);`,
1566+
` }`,
1567+
`}`,
1568+
]);
1569+
});
1570+
1571+
it('should handle initializers referencing other parameters through "this"', async () => {
1572+
writeFile(
1573+
'/dir.ts',
1574+
[
1575+
`import { Directive, Optional } from '@angular/core';`,
1576+
`import { Foo, Bar } from './providers';`,
1577+
``,
1578+
`function createFoo(bar: Bar) { return new Foo(bar); }`,
1579+
``,
1580+
`@Directive()`,
1581+
`class MyDir {`,
1582+
` constructor(private bar: Bar, @Optional() private foo: Foo = createFoo(this.bar)) {}`,
1583+
`}`,
1584+
].join('\n'),
1585+
);
1586+
1587+
await runMigration();
1588+
1589+
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
1590+
`import { Directive, inject } from '@angular/core';`,
1591+
`import { Foo, Bar } from './providers';`,
1592+
``,
1593+
`function createFoo(bar: Bar) { return new Foo(bar); }`,
1594+
``,
1595+
`@Directive()`,
1596+
`class MyDir {`,
1597+
` private bar = inject(Bar);`,
1598+
` private foo = inject(Foo, { optional: true }) ?? createFoo(this.bar);`,
1599+
`}`,
1600+
]);
1601+
});
1602+
1603+
it('should handle parameters with initializers referenced inside super()', async () => {
1604+
writeFile(
1605+
'/dir.ts',
1606+
[
1607+
`import { Directive, Optional } from '@angular/core';`,
1608+
`import { Foo, Bar } from './providers';`,
1609+
`import { Parent } from './parent';`,
1610+
``,
1611+
`function createFoo(bar: Bar) { return new Foo(bar); }`,
1612+
``,
1613+
`@Directive()`,
1614+
`class MyDir extends Parent {`,
1615+
` constructor(bar: Bar, @Optional() private foo: Foo = createFoo(bar)) {`,
1616+
` super(foo);`,
1617+
` }`,
1618+
`}`,
1619+
].join('\n'),
1620+
);
1621+
1622+
await runMigration();
1623+
1624+
expect(tree.readContent('/dir.ts').split('\n')).toEqual([
1625+
`import { Directive, inject } from '@angular/core';`,
1626+
`import { Foo, Bar } from './providers';`,
1627+
`import { Parent } from './parent';`,
1628+
``,
1629+
`function createFoo(bar: Bar) { return new Foo(bar); }`,
1630+
``,
1631+
`@Directive()`,
1632+
`class MyDir extends Parent {`,
1633+
` private foo: Foo;`,
1634+
``,
1635+
` constructor() {`,
1636+
` const bar = inject(Bar);`,
1637+
` const foo = inject(Foo, { optional: true }) ?? createFoo(bar);`,
1638+
``,
1639+
` super(foo);`,
1640+
` this.foo = foo;`,
1641+
``,
1642+
` }`,
1643+
`}`,
1644+
]);
1645+
});
1646+
15041647
describe('internal-only behavior', () => {
15051648
function runInternalMigration() {
15061649
return runMigration({_internalCombineMemberInitializers: true});

0 commit comments

Comments
 (0)