Skip to content

Inject migration follow-up fixes #57367

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
crisbeto wants to merge 2 commits into from
Closed

Inject migration follow-up fixes #57367

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b7b1a19
fix(migrations): preserve optional parameters
crisbeto Aug 13, 2024
99993bd
fix(migrations): avoid duplicating comments when generating properties
crisbeto Aug 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 24 additions & 8 deletions packages/core/schematics/ng-generate/inject-migration/migration.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,8 @@ export function migrateFile(sourceFile: ts.SourceFile, options: MigrationOptions
return;
}

const newProperty = ts.factory.updatePropertyDeclaration(
property,
property.modifiers,
const newProperty = ts.factory.createPropertyDeclaration(
cloneModifiers(property.modifiers),
property.name,
property.questionToken,
property.type,
Expand Down Expand Up @@ -321,12 +320,17 @@ function migrateParameter(
// If the parameter declares a property, we need to declare it (e.g. `private foo: Foo`).
if (declaresProp) {
const prop = ts.factory.createPropertyDeclaration(
node.modifiers?.filter((modifier) => {
// Strip out the DI decorators, as well as `public` which is redundant.
return !ts.isDecorator(modifier) && modifier.kind !== ts.SyntaxKind.PublicKeyword;
}),
cloneModifiers(
node.modifiers?.filter((modifier) => {
// Strip out the DI decorators, as well as `public` which is redundant.
return !ts.isDecorator(modifier) && modifier.kind !== ts.SyntaxKind.PublicKeyword;
}),
),
name,
undefined,
// Don't add the question token to private properties since it won't affect interface implementation.
node.modifiers?.some((modifier) => modifier.kind === ts.SyntaxKind.PrivateKeyword)
? undefined
: node.questionToken,
// We can't initialize the property if it's referenced within a `super` call.
// See the logic further below for the initialization.
usedInSuper ? node.type : undefined,
Expand Down Expand Up @@ -610,3 +614,15 @@ function replaceNodePlaceholder(
const result = printer.printNode(ts.EmitHint.Unspecified, node, sourceFile);
return result.replace(PLACEHOLDER, replacement);
}

/**
* Clones an optional array of modifiers. Can be useful to
* strip the comments from a node with modifiers.
*/
function cloneModifiers(modifiers: ts.ModifierLike[] | ts.NodeArray<ts.ModifierLike> | undefined) {
return modifiers?.map((modifier) => {
return ts.isDecorator(modifier)
? ts.factory.createDecorator(modifier.expression)
: ts.factory.createModifier(modifier.kind);
});
}
89 changes: 89 additions & 0 deletions packages/core/schematics/test/inject_migration_spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1341,6 +1341,60 @@ describe('inject migration', () => {
]);
});

it('should mark optional members if they correspond to optional parameters', async () => {
writeFile(
'/dir.ts',
[
`import { Directive, Optional } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` constructor(@Optional() public foo?: Foo) {}`,
`}`,
].join('\n'),
);

await runMigration();

expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` foo? = inject(Foo, { optional: true });`,
`}`,
]);
});

it('should not mark private members as optional', async () => {
writeFile(
'/dir.ts',
[
`import { Directive, Optional } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` constructor(@Optional() private foo?: Foo) {}`,
`}`,
].join('\n'),
);

await runMigration();

expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo, { optional: true });`,
`}`,
]);
});

describe('internal-only behavior', () => {
function runInternalMigration() {
return runMigration({_internalCombineMemberInitializers: true});
Expand Down Expand Up @@ -1719,5 +1773,40 @@ describe('inject migration', () => {
`}`,
]);
});

it('should account for doc strings when inlining initializers', async () => {
writeFile(
'/dir.ts',
[
`import { Directive } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` /** Value of Foo */`,
` private value: number;`,
``,
` constructor(private foo: Foo) {`,
` this.value = this.foo.getValue();`,
` }`,
`}`,
].join('\n'),
);

await runInternalMigration();

expect(tree.readContent('/dir.ts').split('\n')).toEqual([
`import { Directive, inject } from '@angular/core';`,
`import { Foo } from 'foo';`,
``,
`@Directive()`,
`class MyDir {`,
` private foo = inject(Foo);`,
``,
` /** Value of Foo */`,
` private value: number = this.foo.getValue();`,
`}`,
]);
});
});
});