Skip to content

Commit 68e5370

Browse files
pkozlowski-opensourcethePunderWoman
pkozlowski-opensource
authored and
thePunderWoman
committed
feat(migrations): remove complete calls for migrated outputs (#57671)
This change removes superflous .complete calls for the migrated outputs. PR Close #57671
1 parent f1bbbea commit 68e5370

File tree

4 files changed

+130
-59
lines changed

4 files changed

+130
-59
lines changed

‎packages/core/schematics/migrations/output-migration/output-migration.spec.ts‎

Lines changed: 51 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -149,8 +149,58 @@ describe('outputs', () => {
149149
this.someChange.next('clicked');
150150
}
151151
152+
someMethod() {
153+
this.someChange.pipe();
154+
}
155+
}
156+
`,
157+
);
158+
});
159+
});
160+
161+
describe('.complete migration', () => {
162+
it('should remove .complete usage for migrated outputs', () => {
163+
verify({
164+
before: `
165+
import {Directive, Output, EventEmitter} from '@angular/core';
166+
167+
@Directive()
168+
export class TestDir {
169+
@Output() someChange = new EventEmitter<string>();
170+
171+
ngOnDestroy() {
172+
this.someChange.complete();
173+
}
174+
}
175+
`,
176+
after: `
177+
import { Directive, output } from '@angular/core';
178+
179+
@Directive()
180+
export class TestDir {
181+
readonly someChange = output<string>();
182+
183+
ngOnDestroy() {
184+
185+
}
186+
}
187+
`,
188+
});
189+
});
190+
191+
it('should _not_ migrate .complete usage outside of expression statements', () => {
192+
verifyNoChange(
193+
`
194+
import {Directive, Output, EventEmitter} from '@angular/core';
195+
196+
@Directive()
197+
export class TestDir {
198+
@Output() someChange = new EventEmitter<string>();
199+
152200
ngOnDestroy() {
153-
this.someChange.complete();
201+
// play it safe and skip replacement for any .complete usage that are not
202+
// trivial expression statements
203+
(this.someChange.complete());
154204
}
155205
}
156206
`,
@@ -188,21 +238,6 @@ describe('outputs', () => {
188238
instance.someChange.pipe();
189239
`);
190240
});
191-
192-
it('should _not_ migrate outputs that are used with .complete', () => {
193-
verifyNoChange(`
194-
import {Directive, Output, EventEmitter, OnDestroy} from '@angular/core';
195-
196-
@Directive()
197-
export class TestDir implements OnDestroy {
198-
@Output() someChange = new EventEmitter<string>();
199-
200-
ngOnDestroy() {
201-
this.someChange.complete();
202-
}
203-
}
204-
`);
205-
});
206241
});
207242
});
208243

‎packages/core/schematics/migrations/output-migration/output-migration.ts‎

Lines changed: 41 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -24,13 +24,15 @@ import {
2424
getUniqueIdForProperty,
2525
isTargetOutputDeclaration,
2626
extractSourceOutputDefinition,
27-
isPotentialProblematicEventEmitterUsage,
27+
isPotentialCompleteCallUsage,
2828
isPotentialNextCallUsage,
29+
isPotentialPipeCallUsage,
2930
} from './output_helpers';
3031
import {
3132
calculateImportReplacements,
32-
calculateDeclarationReplacements,
33+
calculateDeclarationReplacement,
3334
calculateNextFnReplacement,
35+
calculateCompleteCallReplacement,
3436
} from './output-replacements';
3537

3638
interface OutputMigrationData {
@@ -73,11 +75,11 @@ export class OutputMigration extends TsurgeFunnelMigration<
7375
const relativePath = projectRelativePath(node.getSourceFile(), projectDirAbsPath);
7476

7577
filesWithOutputDeclarations.add(relativePath);
76-
addOutputReplacements(
78+
addOutputReplacement(
7779
outputFieldReplacements,
7880
outputDef.id,
7981
relativePath,
80-
calculateDeclarationReplacements(projectDirAbsPath, node, outputDef.aliasParam),
82+
calculateDeclarationReplacement(projectDirAbsPath, node, outputDef.aliasParam),
8183
);
8284
}
8385
}
@@ -93,16 +95,43 @@ export class OutputMigration extends TsurgeFunnelMigration<
9395
if (propertyDeclaration !== null) {
9496
const id = getUniqueIdForProperty(projectDirAbsPath, propertyDeclaration);
9597
const relativePath = projectRelativePath(node.getSourceFile(), projectDirAbsPath);
96-
addOutputReplacements(outputFieldReplacements, id, relativePath, [
98+
addOutputReplacement(
99+
outputFieldReplacements,
100+
id,
101+
relativePath,
97102
calculateNextFnReplacement(projectDirAbsPath, node.expression.name),
98-
]);
103+
);
104+
}
105+
}
106+
107+
// detect .complete usages that should be removed
108+
if (isPotentialCompleteCallUsage(node) && ts.isPropertyAccessExpression(node.expression)) {
109+
const propertyDeclaration = isTargetOutputDeclaration(
110+
node.expression.expression,
111+
checker,
112+
reflector,
113+
dtsReader,
114+
);
115+
if (propertyDeclaration !== null) {
116+
const id = getUniqueIdForProperty(projectDirAbsPath, propertyDeclaration);
117+
const relativePath = projectRelativePath(node.getSourceFile(), projectDirAbsPath);
118+
if (ts.isExpressionStatement(node.parent)) {
119+
addOutputReplacement(
120+
outputFieldReplacements,
121+
id,
122+
relativePath,
123+
calculateCompleteCallReplacement(projectDirAbsPath, node.parent),
124+
);
125+
} else {
126+
problematicUsages[id] = true;
127+
}
99128
}
100129
}
101130

102131
// detect unsafe access of the output property
103-
if (isPotentialProblematicEventEmitterUsage(node)) {
132+
if (isPotentialPipeCallUsage(node) && ts.isPropertyAccessExpression(node.expression)) {
104133
const propertyDeclaration = isTargetOutputDeclaration(
105-
node.expression,
134+
node.expression.expression,
106135
checker,
107136
reflector,
108137
dtsReader,
@@ -202,19 +231,19 @@ export class OutputMigration extends TsurgeFunnelMigration<
202231
}
203232
}
204233

205-
function addOutputReplacements(
234+
function addOutputReplacement(
206235
outputFieldReplacements: Record<OutputID, OutputMigrationData>,
207236
outputId: OutputID,
208237
relativePath: ProjectRelativePath,
209-
replacements: Replacement[],
238+
replacement: Replacement,
210239
): void {
211240
const existingReplacements = outputFieldReplacements[outputId];
212241
if (existingReplacements !== undefined) {
213-
existingReplacements.replacements.push(...replacements);
242+
existingReplacements.replacements.push(replacement);
214243
} else {
215244
outputFieldReplacements[outputId] = {
216245
path: relativePath,
217-
replacements: replacements,
246+
replacements: [replacement],
218247
};
219248
}
220249
}

‎packages/core/schematics/migrations/output-migration/output-replacements.ts‎

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -19,11 +19,11 @@ import {applyImportManagerChanges} from '../../utils/tsurge/helpers/apply_import
1919

2020
const printer = ts.createPrinter();
2121

22-
export function calculateDeclarationReplacements(
22+
export function calculateDeclarationReplacement(
2323
projectDirAbsPath: AbsoluteFsPath,
2424
node: ts.PropertyDeclaration,
2525
aliasParam?: ts.Expression,
26-
): Replacement[] {
26+
): Replacement {
2727
const sf = node.getSourceFile();
2828
const payloadTypes =
2929
node.initializer !== undefined && ts.isNewExpression(node.initializer)
@@ -53,16 +53,11 @@ export function calculateDeclarationReplacements(
5353
outputCall,
5454
);
5555

56-
return [
57-
new Replacement(
58-
projectRelativePath(sf, projectDirAbsPath),
59-
new TextUpdate({
60-
position: node.getStart(),
61-
end: node.getEnd(),
62-
toInsert: printer.printNode(ts.EmitHint.Unspecified, updatedOutputDeclaration, sf),
63-
}),
64-
),
65-
];
56+
return prepareTextReplacement(
57+
projectDirAbsPath,
58+
node,
59+
printer.printNode(ts.EmitHint.Unspecified, updatedOutputDeclaration, sf),
60+
);
6661
}
6762

6863
export function calculateImportReplacements(
@@ -103,14 +98,29 @@ export function calculateImportReplacements(
10398
export function calculateNextFnReplacement(
10499
projectDirAbsPath: AbsoluteFsPath,
105100
node: ts.MemberName,
101+
): Replacement {
102+
return prepareTextReplacement(projectDirAbsPath, node, 'emit');
103+
}
104+
105+
export function calculateCompleteCallReplacement(
106+
projectDirAbsPath: AbsoluteFsPath,
107+
node: ts.ExpressionStatement,
108+
): Replacement {
109+
return prepareTextReplacement(projectDirAbsPath, node, '');
110+
}
111+
112+
function prepareTextReplacement(
113+
projectDirAbsPath: AbsoluteFsPath,
114+
node: ts.Node,
115+
replacement: string,
106116
): Replacement {
107117
const sf = node.getSourceFile();
108118
return new Replacement(
109119
projectRelativePath(sf, projectDirAbsPath),
110120
new TextUpdate({
111121
position: node.getStart(),
112122
end: node.getEnd(),
113-
toInsert: 'emit',
123+
toInsert: replacement,
114124
}),
115125
);
116126
}

‎packages/core/schematics/migrations/output-migration/output_helpers.ts‎

Lines changed: 15 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -28,8 +28,6 @@ export interface ExtractedOutput {
2828
aliasParam?: ts.Expression;
2929
}
3030

31-
const PROBLEMATIC_OUTPUT_USAGES = new Set(['complete', 'pipe']);
32-
3331
/**
3432
* Determines if the given node refers to a decorator-based output, and
3533
* returns its resolved metadata if possible.
@@ -60,29 +58,28 @@ function isOutputDeclarationEligibleForMigration(node: ts.PropertyDeclaration) {
6058
);
6159
}
6260

63-
export function isPotentialProblematicEventEmitterUsage(
64-
node: ts.Node,
65-
): node is ts.PropertyAccessExpression {
66-
return (
67-
ts.isPropertyAccessExpression(node) &&
68-
ts.isIdentifier(node.name) &&
69-
PROBLEMATIC_OUTPUT_USAGES.has(node.name.text)
70-
);
71-
}
72-
73-
export function isPotentialNextCallUsage(node: ts.Node): node is ts.CallExpression {
61+
function isPotentialOutputCallUsage(node: ts.Node, name: string): node is ts.CallExpression {
7462
if (
7563
ts.isCallExpression(node) &&
7664
ts.isPropertyAccessExpression(node.expression) &&
7765
ts.isIdentifier(node.expression.name)
7866
) {
79-
const methodName = node.expression.name.text;
80-
if (methodName === 'next') {
81-
return true;
82-
}
67+
return node.expression?.name.text === name;
68+
} else {
69+
return false;
8370
}
71+
}
72+
73+
export function isPotentialPipeCallUsage(node: ts.Node): node is ts.CallExpression {
74+
return isPotentialOutputCallUsage(node, 'pipe');
75+
}
76+
77+
export function isPotentialNextCallUsage(node: ts.Node): node is ts.CallExpression {
78+
return isPotentialOutputCallUsage(node, 'next');
79+
}
8480

85-
return false;
81+
export function isPotentialCompleteCallUsage(node: ts.Node): node is ts.CallExpression {
82+
return isPotentialOutputCallUsage(node, 'complete');
8683
}
8784

8885
export function isTargetOutputDeclaration(

0 commit comments

Comments
 (0)