Skip to content

Commit cbec46a

Browse files
pkozlowski-opensourcethePunderWoman
pkozlowski-opensource
authored and
thePunderWoman
committed
feat(migrations): migrate .pipe calls in outputs used in tests (#57691)
This change uses the outputToObservable utility function to convert migrated outputs to Observables. This happens only in test files as it is a common practice to use RxJS to listen to events raised by the component under test. PR Close #57691
1 parent 3a264db commit cbec46a

File tree

4 files changed

+143
-35
lines changed

4 files changed

+143
-35
lines changed

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

Lines changed: 61 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88

99
import {initMockFileSystem} from '../../../../compiler-cli/src/ngtsc/file_system/testing';
1010
import {runTsurgeMigration} from '../../utils/tsurge/testing';
11+
import {diffText} from '../../utils/tsurge/testing/diff';
1112
import {absoluteFrom} from '@angular/compiler-cli';
1213
import {OutputMigration} from './output-migration';
1314

@@ -320,35 +321,68 @@ describe('outputs', () => {
320321
});
321322
});
322323

323-
describe('declarations _with_ problematic access patterns', () => {
324-
it('should _not_ migrate outputs that are used with .pipe', async () => {
325-
await verifyNoChange(`
326-
import {Directive, Output, EventEmitter} from '@angular/core';
327-
328-
@Directive()
329-
export class TestDir {
330-
@Output() someChange = new EventEmitter();
331-
332-
someMethod() {
333-
this.someChange.pipe();
334-
}
335-
}
336-
`);
324+
describe('.pipe migration', () => {
325+
describe('in test files', () => {
326+
it('should convert to observable in a test file importing jasmine', async () => {
327+
await verify({
328+
before: `
329+
import {Directive, Output, EventEmitter} from '@angular/core';
330+
import {map} from 'rxjs';
331+
import 'jasmine';
332+
333+
@Directive()
334+
export class TestDir {
335+
@Output() someChange = new EventEmitter<number>();
336+
someChange$ = this.someChange.pipe(map((c) => c + 1)).pipe(map((d) => d - 1));
337+
}
338+
`,
339+
after: `
340+
import { outputToObservable } from "@angular/core/rxjs-interop";
341+
342+
import {Directive, output} from '@angular/core';
343+
import {map} from 'rxjs';
344+
import 'jasmine';
345+
346+
@Directive()
347+
export class TestDir {
348+
readonly someChange = output<number>();
349+
someChange$ = outputToObservable(this.someChange).pipe(map((c) => c + 1)).pipe(map((d) => d - 1));
350+
}
351+
`,
352+
});
353+
});
337354
});
338355

339-
it('should _not_ migrate outputs that are used with .pipe outside of a component class', async () => {
340-
await verifyNoChange(`
341-
import {Directive, Output, EventEmitter} from '@angular/core';
342-
343-
@Directive()
344-
export class TestDir {
345-
@Output() someChange = new EventEmitter();
346-
}
347-
348-
let instance: TestDir;
356+
describe('declarations _with_ problematic access patterns', () => {
357+
it('should _not_ migrate outputs that are used with .pipe', () => {
358+
verifyNoChange(`
359+
import {Directive, Output, EventEmitter} from '@angular/core';
360+
361+
@Directive()
362+
export class TestDir {
363+
@Output() someChange = new EventEmitter();
364+
365+
someMethod() {
366+
this.someChange.pipe();
367+
}
368+
}
369+
`);
370+
});
349371

350-
instance.someChange.pipe();
351-
`);
372+
it('should _not_ migrate outputs that are used with .pipe outside of a component class', () => {
373+
verifyNoChange(`
374+
import {Directive, Output, EventEmitter} from '@angular/core';
375+
376+
@Directive()
377+
export class TestDir {
378+
@Output() someChange = new EventEmitter();
379+
}
380+
381+
let instance: TestDir;
382+
383+
instance.someChange.pipe();
384+
`);
385+
});
352386
});
353387
});
354388
});
@@ -386,7 +420,7 @@ async function verify(testCase: {before: string; after: string}) {
386420
const actual = fs.readFile(absoluteFrom('/app.component.ts')).trim();
387421
const expected = testCase.after.trim();
388422

389-
expect(actual).toBe(expected);
423+
expect(actual).withContext(diffText(expected, actual)).toEqual(expected);
390424
}
391425

392426
function populateDeclarationTestCase(declaration: string): string {

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

Lines changed: 27 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,14 @@ import {
2828
isPotentialCompleteCallUsage,
2929
isPotentialNextCallUsage,
3030
isPotentialPipeCallUsage,
31+
isTestRunnerImport,
3132
} from './output_helpers';
3233
import {
3334
calculateImportReplacements,
3435
calculateDeclarationReplacement,
3536
calculateNextFnReplacement,
3637
calculateCompleteCallReplacement,
38+
calculatePipeCallReplacement,
3739
} from './output-replacements';
3840

3941
interface OutputMigrationData {
@@ -62,6 +64,8 @@ export class OutputMigration extends TsurgeFunnelMigration<
6264
const reflector = new TypeScriptReflectionHost(checker);
6365
const dtsReader = new DtsMetadataReader(checker, reflector);
6466

67+
let isTestFile = false;
68+
6569
const outputMigrationVisitor = (node: ts.Node) => {
6670
// detect output declarations
6771
if (ts.isPropertyDeclaration(node)) {
@@ -123,6 +127,11 @@ export class OutputMigration extends TsurgeFunnelMigration<
123127
}
124128
}
125129

130+
// detect imports of test runners
131+
if (isTestRunnerImport(node)) {
132+
isTestFile = true;
133+
}
134+
126135
// detect unsafe access of the output property
127136
if (isPotentialPipeCallUsage(node) && ts.isPropertyAccessExpression(node.expression)) {
128137
const propertyDeclaration = isTargetOutputDeclaration(
@@ -133,7 +142,17 @@ export class OutputMigration extends TsurgeFunnelMigration<
133142
);
134143
if (propertyDeclaration !== null) {
135144
const id = getUniqueIdForProperty(info, propertyDeclaration);
136-
problematicUsages[id] = true;
145+
if (isTestFile) {
146+
const outputFile = projectFile(node.getSourceFile(), info);
147+
addOutputReplacement(
148+
outputFieldReplacements,
149+
id,
150+
outputFile,
151+
...calculatePipeCallReplacement(info, node),
152+
);
153+
} else {
154+
problematicUsages[id] = true;
155+
}
137156
}
138157
}
139158

@@ -142,6 +161,7 @@ export class OutputMigration extends TsurgeFunnelMigration<
142161

143162
// calculate output migration replacements
144163
for (const sf of sourceFiles) {
164+
isTestFile = false;
145165
ts.forEachChild(sf, outputMigrationVisitor);
146166
}
147167

@@ -225,15 +245,14 @@ function addOutputReplacement(
225245
outputFieldReplacements: Record<OutputID, OutputMigrationData>,
226246
outputId: OutputID,
227247
file: ProjectFile,
228-
replacement: Replacement,
248+
...replacements: Replacement[]
229249
): void {
230-
const existingReplacements = outputFieldReplacements[outputId];
231-
if (existingReplacements !== undefined) {
232-
existingReplacements.replacements.push(replacement);
233-
} else {
234-
outputFieldReplacements[outputId] = {
250+
let existingReplacements = outputFieldReplacements[outputId];
251+
if (existingReplacements === undefined) {
252+
outputFieldReplacements[outputId] = existingReplacements = {
235253
file: file,
236-
replacements: [replacement],
254+
replacements: [],
237255
};
238256
}
257+
existingReplacements.replacements.push(...replacements);
239258
}

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

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,53 @@ export function calculateCompleteCallReplacement(
9898
return prepareTextReplacement(info, node, '', node.getFullStart());
9999
}
100100

101+
export function calculatePipeCallReplacement(
102+
info: ProgramInfo,
103+
node: ts.CallExpression,
104+
): Replacement[] {
105+
if (ts.isPropertyAccessExpression(node.expression)) {
106+
const sf = node.getSourceFile();
107+
const importManager = new ImportManager();
108+
109+
const outputToObservableIdent = importManager.addImport({
110+
requestedFile: sf,
111+
exportModuleSpecifier: '@angular/core/rxjs-interop',
112+
exportSymbolName: 'outputToObservable',
113+
});
114+
const toObsCallExp = ts.factory.createCallExpression(outputToObservableIdent, undefined, [
115+
node.expression.expression,
116+
]);
117+
const pipePropAccessExp = ts.factory.updatePropertyAccessExpression(
118+
node.expression,
119+
toObsCallExp,
120+
node.expression.name,
121+
);
122+
const pipeCallExp = ts.factory.updateCallExpression(
123+
node,
124+
pipePropAccessExp,
125+
[],
126+
node.arguments,
127+
);
128+
129+
const replacements = [
130+
prepareTextReplacement(
131+
info,
132+
node,
133+
printer.printNode(ts.EmitHint.Unspecified, pipeCallExp, sf),
134+
),
135+
];
136+
137+
applyImportManagerChanges(importManager, replacements, [sf], info);
138+
139+
return replacements;
140+
} else {
141+
// TODO: assert instead?
142+
throw new Error(
143+
`Unexpected call expression for .pipe - expected a property access but got "${node.getText()}"`,
144+
);
145+
}
146+
}
147+
101148
function prepareTextReplacement(
102149
info: ProgramInfo,
103150
node: ts.Node,

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -154,3 +154,11 @@ export function getUniqueIdForProperty(info: ProgramInfo, prop: ts.PropertyDecla
154154
id.replace(/\.d\.ts$/, '.ts');
155155
return `${id}@@${prop.parent.name ?? 'unknown-class'}@@${prop.name.getText()}` as OutputID;
156156
}
157+
158+
export function isTestRunnerImport(node: ts.Node) {
159+
if (ts.isImportDeclaration(node)) {
160+
const moduleSpecifier = node.moduleSpecifier.getText();
161+
return moduleSpecifier.includes('jasmine') || moduleSpecifier.includes('catalyst');
162+
}
163+
return false;
164+
}

0 commit comments

Comments
 (0)