Skip to content

Commit 3a264db

Browse files
pkozlowski-opensourcethePunderWoman
pkozlowski-opensource
authored and
thePunderWoman
committed
fix(migrations): properly handle comments in output migration (#57691)
This change contains a fix that takes into account comments attached to outputs and preserves / removes those as needed. PR Close #57691
1 parent 40ff18f commit 3a264db

File tree

2 files changed

+164
-51
lines changed

2 files changed

+164
-51
lines changed

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

Lines changed: 160 additions & 47 deletions
Original file line numberDiff line numberDiff line change
@@ -18,36 +18,36 @@ describe('outputs', () => {
1818

1919
describe('outputs migration', () => {
2020
describe('EventEmitter declarations without problematic access patterns', () => {
21-
it('should migrate declaration with a primitive type hint', () => {
22-
verifyDeclaration({
21+
it('should migrate declaration with a primitive type hint', async () => {
22+
await verifyDeclaration({
2323
before: '@Output() readonly someChange = new EventEmitter<string>();',
2424
after: 'readonly someChange = output<string>();',
2525
});
2626
});
2727

28-
it('should migrate declaration with complex type hint', () => {
29-
verifyDeclaration({
28+
it('should migrate declaration with complex type hint', async () => {
29+
await verifyDeclaration({
3030
before: '@Output() readonly someChange = new EventEmitter<string | number>();',
3131
after: 'readonly someChange = output<string | number>();',
3232
});
3333
});
3434

35-
it('should migrate declaration without type hint', () => {
36-
verifyDeclaration({
35+
it('should migrate declaration without type hint', async () => {
36+
await verifyDeclaration({
3737
before: '@Output() readonly someChange = new EventEmitter();',
3838
after: 'readonly someChange = output();',
3939
});
4040
});
4141

42-
it('should take alias into account', () => {
43-
verifyDeclaration({
42+
it('should take alias into account', async () => {
43+
await verifyDeclaration({
4444
before: `@Output({alias: 'otherChange'}) readonly someChange = new EventEmitter();`,
4545
after: `readonly someChange = output({ alias: 'otherChange' });`,
4646
});
4747
});
4848

49-
it('should support alias as statically analyzable reference', () => {
50-
verify({
49+
it('should support alias as statically analyzable reference', async () => {
50+
await verify({
5151
before: `
5252
import {Directive, Output, EventEmitter} from '@angular/core';
5353
@@ -71,44 +71,128 @@ describe('outputs', () => {
7171
});
7272
});
7373

74-
it('should add readonly modifier', () => {
75-
verifyDeclaration({
74+
it('should add readonly modifier', async () => {
75+
await verifyDeclaration({
7676
before: '@Output() someChange = new EventEmitter();',
7777
after: 'readonly someChange = output();',
7878
});
7979
});
8080

81-
it('should respect visibility modifiers', () => {
82-
verifyDeclaration({
81+
it('should respect visibility modifiers', async () => {
82+
await verifyDeclaration({
8383
before: `@Output() protected someChange = new EventEmitter();`,
8484
after: `protected readonly someChange = output();`,
8585
});
8686
});
8787

88-
it('should migrate multiple outputs', () => {
89-
// TODO: whitespace are messing up test verification
90-
verifyDeclaration({
91-
before: `@Output() someChange1 = new EventEmitter();
92-
@Output() someChange2 = new EventEmitter();`,
93-
after: `readonly someChange1 = output();
94-
readonly someChange2 = output();`,
88+
it('should preserve single line JSdoc comments when migrating outputs', async () => {
89+
await verify({
90+
before: `
91+
import {Directive, Output, EventEmitter} from '@angular/core';
92+
93+
@Directive()
94+
export class TestDir {
95+
/** Whenever there is change, emits an event. */
96+
@Output() someChange = new EventEmitter();
97+
}
98+
`,
99+
after: `
100+
import {Directive, output} from '@angular/core';
101+
102+
@Directive()
103+
export class TestDir {
104+
/** Whenever there is change, emits an event. */
105+
readonly someChange = output();
106+
}
107+
`,
108+
});
109+
});
110+
111+
it('should preserve multiline JSdoc comments when migrating outputs', async () => {
112+
await verify({
113+
before: `
114+
import {Directive, Output, EventEmitter} from '@angular/core';
115+
116+
@Directive()
117+
export class TestDir {
118+
/**
119+
* Whenever there is change, emits an event.
120+
*/
121+
@Output() someChange = new EventEmitter();
122+
}
123+
`,
124+
after: `
125+
import {Directive, output} from '@angular/core';
126+
127+
@Directive()
128+
export class TestDir {
129+
/**
130+
* Whenever there is change, emits an event.
131+
*/
132+
readonly someChange = output();
133+
}
134+
`,
135+
});
136+
});
137+
138+
it('should preserve multiline comments when migrating outputs', async () => {
139+
await verify({
140+
before: `
141+
import {Directive, Output, EventEmitter} from '@angular/core';
142+
143+
@Directive()
144+
export class TestDir {
145+
/* Whenever there is change,emits an event. */
146+
@Output() someChange = new EventEmitter();
147+
}
148+
`,
149+
after: `
150+
import {Directive, output} from '@angular/core';
151+
152+
@Directive()
153+
export class TestDir {
154+
/* Whenever there is change,emits an event. */
155+
readonly someChange = output();
156+
}
157+
`,
158+
});
159+
});
160+
161+
it('should migrate multiple outputs', async () => {
162+
await verifyDeclaration({
163+
before:
164+
'@Output() someChange1 = new EventEmitter();\n@Output() someChange2 = new EventEmitter();',
165+
after: `readonly someChange1 = output();\nreadonly someChange2 = output();`,
95166
});
96167
});
97168

98-
it('should migrate only EventEmitter outputs when multiple outputs exist', () => {
99-
// TODO: whitespace are messing up test verification
100-
verifyDeclaration({
101-
before: `@Output() someChange1 = new EventEmitter();
102-
@Output() someChange2 = new Subject();`,
103-
after: `readonly someChange1 = output();
104-
@Output() someChange2 = new Subject();`,
169+
it('should migrate only EventEmitter outputs when multiple outputs exist', async () => {
170+
await verify({
171+
before: `
172+
import {Directive, Output, EventEmitter, Subject} from '@angular/core';
173+
174+
@Directive()
175+
export class TestDir {
176+
@Output() someChange1 = new EventEmitter();
177+
@Output() someChange2 = new Subject();
178+
}
179+
`,
180+
after: `
181+
import {Directive, Subject, output} from '@angular/core';
182+
183+
@Directive()
184+
export class TestDir {
185+
readonly someChange1 = output();
186+
@Output() someChange2 = new Subject();
187+
}
188+
`,
105189
});
106190
});
107191
});
108192

109193
describe('.next migration', () => {
110-
it('should migrate .next usages that should have been .emit', () => {
111-
verify({
194+
it('should migrate .next usages that should have been .emit', async () => {
195+
await verify({
112196
before: `
113197
import {Directive, Output, EventEmitter} from '@angular/core';
114198
@@ -136,8 +220,8 @@ describe('outputs', () => {
136220
});
137221
});
138222

139-
it('should _not_ migrate .next usages when problematic output usage is detected', () => {
140-
verifyNoChange(
223+
it('should _not_ migrate .next usages when problematic output usage is detected', async () => {
224+
await verifyNoChange(
141225
`
142226
import {Directive, Output, EventEmitter} from '@angular/core';
143227
@@ -159,8 +243,36 @@ describe('outputs', () => {
159243
});
160244

161245
describe('.complete migration', () => {
162-
it('should remove .complete usage for migrated outputs', () => {
163-
verify({
246+
it('should remove .complete usage for migrated outputs', async () => {
247+
await verify({
248+
before: `
249+
import {Directive, Output, EventEmitter} from '@angular/core';
250+
251+
@Directive()
252+
export class TestDir {
253+
@Output() someChange = new EventEmitter<string>();
254+
255+
ngOnDestroy() {
256+
this.someChange.complete();
257+
}
258+
}
259+
`,
260+
after: `
261+
import {Directive, output} from '@angular/core';
262+
263+
@Directive()
264+
export class TestDir {
265+
readonly someChange = output<string>();
266+
267+
ngOnDestroy() {
268+
}
269+
}
270+
`,
271+
});
272+
});
273+
274+
it('should remove .complete usage with comments', async () => {
275+
await verify({
164276
before: `
165277
import {Directive, Output, EventEmitter} from '@angular/core';
166278
@@ -169,6 +281,7 @@ describe('outputs', () => {
169281
@Output() someChange = new EventEmitter<string>();
170282
171283
ngOnDestroy() {
284+
// maybe complete before the destroy?
172285
this.someChange.complete();
173286
}
174287
}
@@ -181,15 +294,14 @@ describe('outputs', () => {
181294
readonly someChange = output<string>();
182295
183296
ngOnDestroy() {
184-
185297
}
186298
}
187299
`,
188300
});
189301
});
190302

191-
it('should _not_ migrate .complete usage outside of expression statements', () => {
192-
verifyNoChange(
303+
it('should _not_ migrate .complete usage outside of expression statements', async () => {
304+
await verifyNoChange(
193305
`
194306
import {Directive, Output, EventEmitter} from '@angular/core';
195307
@@ -209,8 +321,8 @@ describe('outputs', () => {
209321
});
210322

211323
describe('declarations _with_ problematic access patterns', () => {
212-
it('should _not_ migrate outputs that are used with .pipe', () => {
213-
verifyNoChange(`
324+
it('should _not_ migrate outputs that are used with .pipe', async () => {
325+
await verifyNoChange(`
214326
import {Directive, Output, EventEmitter} from '@angular/core';
215327
216328
@Directive()
@@ -224,8 +336,8 @@ describe('outputs', () => {
224336
`);
225337
});
226338

227-
it('should _not_ migrate outputs that are used with .pipe outside of a component class', () => {
228-
verifyNoChange(`
339+
it('should _not_ migrate outputs that are used with .pipe outside of a component class', async () => {
340+
await verifyNoChange(`
229341
import {Directive, Output, EventEmitter} from '@angular/core';
230342
231343
@Directive()
@@ -242,8 +354,8 @@ describe('outputs', () => {
242354
});
243355

244356
describe('declarations other than EventEmitter', () => {
245-
it('should _not_ migrate outputs that are initialized with sth else than EventEmitter', () => {
246-
verify({
357+
it('should _not_ migrate outputs that are initialized with sth else than EventEmitter', async () => {
358+
await verify({
247359
before: populateDeclarationTestCase('@Output() protected someChange = new Subject();'),
248360
after: populateDeclarationTestCase('@Output() protected someChange = new Subject();'),
249361
});
@@ -252,14 +364,14 @@ describe('outputs', () => {
252364
});
253365

254366
async function verifyDeclaration(testCase: {before: string; after: string}) {
255-
verify({
367+
await verify({
256368
before: populateDeclarationTestCase(testCase.before),
257369
after: populateExpectedResult(testCase.after),
258370
});
259371
}
260372

261373
async function verifyNoChange(beforeAndAfter: string) {
262-
verify({before: beforeAndAfter, after: beforeAndAfter});
374+
await verify({before: beforeAndAfter, after: beforeAndAfter});
263375
}
264376

265377
async function verify(testCase: {before: string; after: string}) {
@@ -271,9 +383,10 @@ async function verify(testCase: {before: string; after: string}) {
271383
},
272384
]);
273385

274-
let actual = fs.readFile(absoluteFrom('/app.component.ts'));
386+
const actual = fs.readFile(absoluteFrom('/app.component.ts')).trim();
387+
const expected = testCase.after.trim();
275388

276-
expect(actual).toBe(testCase.after);
389+
expect(actual).toBe(expected);
277390
}
278391

279392
function populateDeclarationTestCase(declaration: string): string {

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

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,7 @@ export function calculateDeclarationReplacement(
3535
(modifier) => !ts.isDecorator(modifier) && modifier.kind !== ts.SyntaxKind.ReadonlyKeyword,
3636
);
3737

38-
const updatedOutputDeclaration = ts.factory.updatePropertyDeclaration(
39-
node,
38+
const updatedOutputDeclaration = ts.factory.createPropertyDeclaration(
4039
// Think: this logic of dealing with modifiers is applicable to all signal-based migrations
4140
ts.factory.createNodeArray([
4241
...existingModifiers,
@@ -96,19 +95,20 @@ export function calculateCompleteCallReplacement(
9695
info: ProgramInfo,
9796
node: ts.ExpressionStatement,
9897
): Replacement {
99-
return prepareTextReplacement(info, node, '');
98+
return prepareTextReplacement(info, node, '', node.getFullStart());
10099
}
101100

102101
function prepareTextReplacement(
103102
info: ProgramInfo,
104103
node: ts.Node,
105104
replacement: string,
105+
start?: number,
106106
): Replacement {
107107
const sf = node.getSourceFile();
108108
return new Replacement(
109109
projectFile(sf, info),
110110
new TextUpdate({
111-
position: node.getStart(),
111+
position: start ?? node.getStart(),
112112
end: node.getEnd(),
113113
toInsert: replacement,
114114
}),

0 commit comments

Comments
 (0)