Skip to content

Commit c468198

Browse files
devversionalxhub
devversion
authored and
alxhub
committed
refactor(migrations): properly handle multi query migration (#57556)
Properly handles queries with multiple results, by extracting the type from the `QueryList`. Also adds more tests and handles imports. PR Close #57556
1 parent 86e6f7b commit c468198

File tree

9 files changed

+379
-67
lines changed

9 files changed

+379
-67
lines changed

‎packages/core/schematics/migrations/signal-migration/src/passes/10_apply_import_manager.ts‎

Lines changed: 2 additions & 58 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,8 @@
88

99
import ts from 'typescript';
1010
import {ImportManager} from '../../../../../../compiler-cli/src/ngtsc/translator';
11+
import {applyImportManagerChanges} from '../../../../utils/tsurge/helpers/apply_import_manager';
1112
import {MigrationResult} from '../result';
12-
import {
13-
absoluteFrom,
14-
absoluteFromSourceFile,
15-
} from '../../../../../../compiler-cli/src/ngtsc/file_system';
16-
import {Replacement, TextUpdate} from '../../../../utils/tsurge/replacement';
1713

1814
/**
1915
* Phase that applies all changes recorded by the import manager in
@@ -24,57 +20,5 @@ export function pass10_applyImportManager(
2420
result: MigrationResult,
2521
sourceFiles: readonly ts.SourceFile[],
2622
) {
27-
const {newImports, updatedImports, deletedImports} = importManager.finalize();
28-
const printer = ts.createPrinter({});
29-
const pathToFile = new Map<string, ts.SourceFile>(sourceFiles.map((s) => [s.fileName, s]));
30-
31-
// Capture new imports
32-
newImports.forEach((newImports, fileName) => {
33-
newImports.forEach((newImport) => {
34-
const printedImport = printer.printNode(
35-
ts.EmitHint.Unspecified,
36-
newImport,
37-
pathToFile.get(fileName)!,
38-
);
39-
result.replacements.push(
40-
new Replacement(
41-
absoluteFrom(fileName),
42-
new TextUpdate({position: 0, end: 0, toInsert: `${printedImport}\n`}),
43-
),
44-
);
45-
});
46-
});
47-
48-
// Capture updated imports
49-
for (const [oldBindings, newBindings] of updatedImports.entries()) {
50-
const printedBindings = printer.printNode(
51-
ts.EmitHint.Unspecified,
52-
newBindings,
53-
oldBindings.getSourceFile(),
54-
);
55-
result.replacements.push(
56-
new Replacement(
57-
absoluteFromSourceFile(oldBindings.getSourceFile()),
58-
new TextUpdate({
59-
position: oldBindings.getStart(),
60-
end: oldBindings.getEnd(),
61-
toInsert: printedBindings,
62-
}),
63-
),
64-
);
65-
}
66-
67-
// Update removed imports
68-
for (const removedImport of deletedImports) {
69-
result.replacements.push(
70-
new Replacement(
71-
absoluteFromSourceFile(removedImport.getSourceFile()),
72-
new TextUpdate({
73-
position: removedImport.getStart(),
74-
end: removedImport.getEnd(),
75-
toInsert: '',
76-
}),
77-
),
78-
);
79-
}
23+
applyImportManagerChanges(importManager, result.replacements, sourceFiles);
8024
}

‎packages/core/schematics/migrations/signal-queries-migration/convert_query_property.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ import {ImportManager} from '../../../../compiler-cli/private/migrations';
1414
import assert from 'assert';
1515
import {WrappedNodeExpr} from '@angular/compiler';
1616
import {removeFromUnionIfPossible} from '../signal-migration/src/utils/remove_from_union';
17+
import {extractQueryListType} from './query_list_type';
1718

1819
const printer = ts.createPrinter();
1920

@@ -58,6 +59,16 @@ export function computeReplacementsToMigrateQuery(
5859
];
5960
let type = node.type;
6061

62+
// For multi queries, attempt to unwrap `QueryList` types, or infer the
63+
// type from the initializer, if possible.
64+
if (!metadata.queryInfo.first) {
65+
if (type === undefined && node.initializer !== undefined) {
66+
type = extractQueryListType(node.initializer);
67+
} else if (type !== undefined) {
68+
type = extractQueryListType(type);
69+
}
70+
}
71+
6172
if (metadata.queryInfo.read !== null) {
6273
assert(metadata.queryInfo.read instanceof WrappedNodeExpr);
6374
optionProperties.push(

‎packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts‎

Lines changed: 186 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,8 @@ interface TestCase {
1919
focus?: boolean;
2020
}
2121

22-
const testCases: TestCase[] = [
22+
const declarationTestCases: TestCase[] = [
23+
// View Child
2324
{
2425
id: 'viewChild with string locator and nullable',
2526
before: `@ViewChild('myBtn') button: MyButton|undefined = undefined;`,
@@ -50,26 +51,154 @@ const testCases: TestCase[] = [
5051
before: `@ViewChild('myBtn', {read: ElementRef}) buttonEl!: ElementRef;`,
5152
after: `readonly buttonEl = viewChild.required('myBtn', { read: ElementRef });`,
5253
},
54+
// Content Child
55+
{
56+
id: 'contentChild with string locator and nullable',
57+
before: `@ContentChild('myBtn') button: MyButton|undefined = undefined;`,
58+
after: `readonly button = contentChild<MyButton>('myBtn');`,
59+
},
60+
{
61+
id: 'contentChild with class type locator and nullable',
62+
before: `@ContentChild(MyButton) button: MyButton|undefined = undefined;`,
63+
after: `readonly button = contentChild(MyButton);`,
64+
},
65+
{
66+
id: 'contentChild with class type locator and nullable via question-mark shorthand',
67+
before: `@ContentChild(MyButton) button?: MyButton;`,
68+
after: `readonly button = contentChild(MyButton);`,
69+
},
70+
{
71+
id: 'contentChild with class type locator and exclamation mark to simulate required',
72+
before: `@ContentChild(MyButton) button!: MyButton;`,
73+
after: `readonly button = contentChild.required(MyButton);`,
74+
},
75+
{
76+
id: 'contentChild with string locator and read option, nullable shorthand',
77+
before: `@ContentChild('myBtn', {read: ElementRef}) buttonEl?: ElementRef;`,
78+
after: `readonly buttonEl = contentChild('myBtn', { read: ElementRef });`,
79+
},
80+
{
81+
id: 'contentChild with string locator and read option, required',
82+
before: `@ContentChild('myBtn', {read: ElementRef}) buttonEl!: ElementRef;`,
83+
after: `readonly buttonEl = contentChild.required('myBtn', { read: ElementRef });`,
84+
},
85+
{
86+
id: 'contentChild with string locator and read option, required',
87+
before: `@ContentChild('myBtn', {read: ElementRef}) buttonEl!: ElementRef;`,
88+
after: `readonly buttonEl = contentChild.required('myBtn', { read: ElementRef });`,
89+
},
90+
{
91+
id: 'contentChild with descendants option',
92+
before: `@ContentChild('myBtn', {descendants: false}) buttonEl!: ElementRef;`,
93+
after: `readonly buttonEl = contentChild.required<ElementRef>('myBtn', { descendants: false });`,
94+
},
95+
// ViewChildren
96+
{
97+
id: 'viewChildren with string locator and nullable',
98+
before: `@ViewChildren('myBtn') button?: QueryList<ElementRef>;`,
99+
after: `readonly button = viewChildren<ElementRef>('myBtn');`,
100+
},
101+
{
102+
id: 'viewChildren with class type locator and nullable',
103+
before: `@ViewChildren(MyButton) button?: QueryList<MyButton>;`,
104+
after: `readonly button = viewChildren(MyButton);`,
105+
},
106+
{
107+
id: 'viewChildren with class type locator and exclamation mark',
108+
before: `@ViewChildren(MyButton) button!: QueryList<MyButton>;`,
109+
after: `readonly button = viewChildren(MyButton);`,
110+
},
111+
{
112+
id: 'viewChild with string locator and read option, nullable shorthand',
113+
before: `@ViewChildren('myBtn', {read: ElementRef}) buttonEl?: QueryList<ElementRef>;`,
114+
after: `readonly buttonEl = viewChildren('myBtn', { read: ElementRef });`,
115+
},
116+
{
117+
id: 'viewChildren with string locator and read option, required',
118+
before: `@ViewChildren('myBtn', {read: ElementRef}) buttonEl!: QueryList<ElementRef>;`,
119+
after: `readonly buttonEl = viewChildren('myBtn', { read: ElementRef });`,
120+
},
121+
{
122+
id: 'viewChildren with query list as initializer value',
123+
before: `@ViewChildren('myBtn') buttonEl = new QueryList<ElementRef>()`,
124+
after: `readonly buttonEl = viewChildren<ElementRef>('myBtn');`,
125+
},
126+
{
127+
id: 'viewChildren with query list as initializer value, and descendants option',
128+
before: `@ViewChildren('myBtn', {descendants: false}) buttonEl = new QueryList<ElementRef>()`,
129+
after: `readonly buttonEl = viewChildren<ElementRef>('myBtn', { descendants: false });`,
130+
},
131+
{
132+
id: 'viewChildren with query list as initializer value, and descendants option but same as default',
133+
before: `@ViewChildren('myBtn', {descendants: true}) buttonEl = new QueryList<ElementRef>()`,
134+
after: `readonly buttonEl = viewChildren<ElementRef>('myBtn');`,
135+
},
136+
// ContentChildren
137+
{
138+
id: 'contentChildren with string locator and nullable',
139+
before: `@ContentChildren('myBtn') button?: QueryList<ElementRef>;`,
140+
after: `readonly button = contentChildren<ElementRef>('myBtn');`,
141+
},
142+
{
143+
id: 'contentChildren with class type locator and nullable',
144+
before: `@ContentChildren(MyButton) button?: QueryList<MyButton>;`,
145+
after: `readonly button = contentChildren(MyButton);`,
146+
},
147+
{
148+
id: 'contentChildren with class type locator and exclamation mark',
149+
before: `@ContentChildren(MyButton) button!: QueryList<MyButton>;`,
150+
after: `readonly button = contentChildren(MyButton);`,
151+
},
152+
{
153+
id: 'contentChildren with string locator and read option, nullable shorthand',
154+
before: `@ContentChildren('myBtn', {read: ElementRef}) buttonEl?: QueryList<ElementRef>;`,
155+
after: `readonly buttonEl = contentChildren('myBtn', { read: ElementRef });`,
156+
},
157+
{
158+
id: 'contentChildren with string locator and read option, required',
159+
before: `@ContentChildren('myBtn', {read: ElementRef}) buttonEl!: QueryList<ElementRef>;`,
160+
after: `readonly buttonEl = contentChildren('myBtn', { read: ElementRef });`,
161+
},
162+
{
163+
id: 'contentChildren with query list as initializer value',
164+
before: `@ContentChildren('myBtn') buttonEl = new QueryList<ElementRef>()`,
165+
after: `readonly buttonEl = contentChildren<ElementRef>('myBtn');`,
166+
},
167+
{
168+
id: 'contentChildren with query list as initializer value, and descendants option',
169+
before: `@ContentChildren('myBtn', {descendants: true}) buttonEl = new QueryList<ElementRef>()`,
170+
after: `readonly buttonEl = contentChildren<ElementRef>('myBtn', { descendants: true });`,
171+
},
172+
{
173+
id: 'contentChildren with query list as initializer value, and descendants option but same as default',
174+
before: `@ContentChildren('myBtn', {descendants: false}) buttonEl = new QueryList<ElementRef>()`,
175+
after: `readonly buttonEl = contentChildren<ElementRef>('myBtn');`,
176+
},
53177
];
54178

55179
describe('signal queries migration', () => {
56180
beforeEach(() => {
57181
initMockFileSystem('Native');
58182
});
59183

60-
describe('test cases', () => {
61-
for (const c of testCases) {
184+
describe('declaration test cases', () => {
185+
for (const c of declarationTestCases) {
62186
(c.focus ? fit : it)(c.id, async () => {
63187
const fs = await runTsurgeMigration(new SignalQueriesMigration(), [
64188
{
65189
name: absoluteFrom('/app.component.ts'),
66190
isProgramRootFile: true,
67-
contents: populateTestCaseComponent(c.before),
191+
contents: populateDeclarationTestCaseComponent(c.before),
68192
},
69193
]);
70194

71-
const actual = fs.readFile(absoluteFrom('/app.component.ts'));
72-
const expected = populateTestCaseComponent(c.after);
195+
let actual = fs.readFile(absoluteFrom('/app.component.ts'));
196+
let expected = populateDeclarationTestCaseComponent(c.after);
197+
198+
// Cut off the string before the class declaration.
199+
// The import diff is irrelevant here for now.
200+
actual = actual.substring(actual.indexOf('@Directive'));
201+
expected = expected.substring(expected.indexOf('@Directive'));
73202

74203
if (actual !== expected) {
75204
expect(diffText(expected, actual)).toBe('');
@@ -102,9 +231,59 @@ describe('signal queries migration', () => {
102231
expect(actual).not.toContain(`viewChild`);
103232
expect(actual).toContain(`@ViewChild('labelRef') label?: ElementRef;`);
104233
});
234+
235+
it('should update imports when migrating', async () => {
236+
const fs = await runTsurgeMigration(new SignalQueriesMigration(), [
237+
{
238+
name: absoluteFrom('/app.component.ts'),
239+
isProgramRootFile: true,
240+
contents: `
241+
import {ViewChild, ElementRef, Directive} from '@angular/core';
242+
243+
@Directive()
244+
class MyComp {
245+
@ViewChild('labelRef') label?: ElementRef;
246+
}
247+
`,
248+
},
249+
]);
250+
251+
const actual = fs.readFile(absoluteFrom('/app.component.ts'));
252+
expect(actual).toContain(`import { ElementRef, Directive, viewChild } from '@angular/core';`);
253+
expect(actual).toContain(`label = viewChild<ElementRef>('labelRef')`);
254+
});
255+
256+
it('should not remove imports when partially migrating', async () => {
257+
const fs = await runTsurgeMigration(new SignalQueriesMigration(), [
258+
{
259+
name: absoluteFrom('/app.component.ts'),
260+
isProgramRootFile: true,
261+
contents: `
262+
import {ViewChild, ElementRef, Directive} from '@angular/core';
263+
264+
@Directive()
265+
class MyComp {
266+
@ViewChild('labelRef') label?: ElementRef;
267+
@ViewChild('labelRef2') label2?: ElementRef;
268+
269+
click() {
270+
this.label2 = undefined;
271+
}
272+
}
273+
`,
274+
},
275+
]);
276+
277+
const actual = fs.readFile(absoluteFrom('/app.component.ts'));
278+
expect(actual).toContain(
279+
`import { ViewChild, ElementRef, Directive, viewChild } from '@angular/core';`,
280+
);
281+
expect(actual).toContain(`label = viewChild<ElementRef>('labelRef')`);
282+
expect(actual).toContain(`@ViewChild('labelRef2') label2?: ElementRef;`);
283+
});
105284
});
106285

107-
function populateTestCaseComponent(declaration: string): string {
286+
function populateDeclarationTestCaseComponent(declaration: string): string {
108287
return `
109288
import {
110289
ViewChild,

0 commit comments

Comments
 (0)