skills/angular__angular
1
0
Fork 0
mirror of https://github.com/angular/angular.git synced 2026-09-14 13:54:52 +08:00
Code Issues Packages Projects Releases Wiki Activity

refactor(migrations): properly handle multi query migration (#57556)

Browse Source 
Properly handles queries with multiple results, by extracting the
type from the `QueryList`.

Also adds more tests and handles imports.

PR Close #57556
This commit is contained in:
Paul Gschwendtner
2024-08-27 11:33:59 +00:00
committed by Alex Rickabaugh
parent 10fce207e0
commit b14c864170
9 changed files with 379 additions and 67 deletions
Download Patch File Download Diff File Expand all files Collapse all files
packages/core/schematics/migrations/signal-migration/src/passes/10_apply_import_manager.ts
+2 -58
View File
@@ -8,12 +8,8 @@
import ts from 'typescript';
import {ImportManager} from '../../../../../../compiler-cli/src/ngtsc/translator';
import {applyImportManagerChanges} from '../../../../utils/tsurge/helpers/apply_import_manager';
import {MigrationResult} from '../result';
import {
absoluteFrom,
absoluteFromSourceFile,
} from '../../../../../../compiler-cli/src/ngtsc/file_system';
import {Replacement, TextUpdate} from '../../../../utils/tsurge/replacement';
/**
* Phase that applies all changes recorded by the import manager in
@@ -24,57 +20,5 @@ export function pass10_applyImportManager(
result: MigrationResult,
sourceFiles: readonly ts.SourceFile[],
) {
const {newImports, updatedImports, deletedImports} = importManager.finalize();
const printer = ts.createPrinter({});
const pathToFile = new Map<string, ts.SourceFile>(sourceFiles.map((s) => [s.fileName, s]));
// Capture new imports
newImports.forEach((newImports, fileName) => {
newImports.forEach((newImport) => {
const printedImport = printer.printNode(
ts.EmitHint.Unspecified,
newImport,
pathToFile.get(fileName)!,
);
result.replacements.push(
new Replacement(
absoluteFrom(fileName),
new TextUpdate({position: 0, end: 0, toInsert: `${printedImport}\n`}),
),
);
});
});
// Capture updated imports
for (const [oldBindings, newBindings] of updatedImports.entries()) {
const printedBindings = printer.printNode(
ts.EmitHint.Unspecified,
newBindings,
oldBindings.getSourceFile(),
);
result.replacements.push(
new Replacement(
absoluteFromSourceFile(oldBindings.getSourceFile()),
new TextUpdate({
position: oldBindings.getStart(),
end: oldBindings.getEnd(),
toInsert: printedBindings,
}),
),
);
}
// Update removed imports
for (const removedImport of deletedImports) {
result.replacements.push(
new Replacement(
absoluteFromSourceFile(removedImport.getSourceFile()),
new TextUpdate({
position: removedImport.getStart(),
end: removedImport.getEnd(),
toInsert: '',
}),
),
);
}
applyImportManagerChanges(importManager, result.replacements, sourceFiles);
}
packages/core/schematics/migrations/signal-queries-migration/convert_query_property.ts
+11
View File
@@ -14,6 +14,7 @@ import {ImportManager} from '../../../../compiler-cli/private/migrations';
import assert from 'assert';
import {WrappedNodeExpr} from '@angular/compiler';
import {removeFromUnionIfPossible} from '../signal-migration/src/utils/remove_from_union';
import {extractQueryListType} from './query_list_type';
const printer = ts.createPrinter();
@@ -58,6 +59,16 @@ export function computeReplacementsToMigrateQuery(
];
let type = node.type;
// For multi queries, attempt to unwrap `QueryList` types, or infer the
// type from the initializer, if possible.
if (!metadata.queryInfo.first) {
if (type === undefined && node.initializer !== undefined) {
type = extractQueryListType(node.initializer);
} else if (type !== undefined) {
type = extractQueryListType(type);
}
}
if (metadata.queryInfo.read !== null) {
assert(metadata.queryInfo.read instanceof WrappedNodeExpr);
optionProperties.push(
packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts
+186 -7
View File
@@ -19,7 +19,8 @@ interface TestCase {
focus?: boolean;
}
const testCases: TestCase[] = [
const declarationTestCases: TestCase[] = [
// View Child
{
id: 'viewChild with string locator and nullable',
before: `@ViewChild('myBtn') button: MyButton|undefined = undefined;`,
@@ -50,6 +51,129 @@ const testCases: TestCase[] = [
before: `@ViewChild('myBtn', {read: ElementRef}) buttonEl!: ElementRef;`,
after: `readonly buttonEl = viewChild.required('myBtn', { read: ElementRef });`,
},
// Content Child
{
id: 'contentChild with string locator and nullable',
before: `@ContentChild('myBtn') button: MyButton|undefined = undefined;`,
after: `readonly button = contentChild<MyButton>('myBtn');`,
},
{
id: 'contentChild with class type locator and nullable',
before: `@ContentChild(MyButton) button: MyButton|undefined = undefined;`,
after: `readonly button = contentChild(MyButton);`,
},
{
id: 'contentChild with class type locator and nullable via question-mark shorthand',
before: `@ContentChild(MyButton) button?: MyButton;`,
after: `readonly button = contentChild(MyButton);`,
},
{
id: 'contentChild with class type locator and exclamation mark to simulate required',
before: `@ContentChild(MyButton) button!: MyButton;`,
after: `readonly button = contentChild.required(MyButton);`,
},
{
id: 'contentChild with string locator and read option, nullable shorthand',
before: `@ContentChild('myBtn', {read: ElementRef}) buttonEl?: ElementRef;`,
after: `readonly buttonEl = contentChild('myBtn', { read: ElementRef });`,
},
{
id: 'contentChild with string locator and read option, required',
before: `@ContentChild('myBtn', {read: ElementRef}) buttonEl!: ElementRef;`,
after: `readonly buttonEl = contentChild.required('myBtn', { read: ElementRef });`,
},
{
id: 'contentChild with string locator and read option, required',
before: `@ContentChild('myBtn', {read: ElementRef}) buttonEl!: ElementRef;`,
after: `readonly buttonEl = contentChild.required('myBtn', { read: ElementRef });`,
},
{
id: 'contentChild with descendants option',
before: `@ContentChild('myBtn', {descendants: false}) buttonEl!: ElementRef;`,
after: `readonly buttonEl = contentChild.required<ElementRef>('myBtn', { descendants: false });`,
},
// ViewChildren
{
id: 'viewChildren with string locator and nullable',
before: `@ViewChildren('myBtn') button?: QueryList<ElementRef>;`,
after: `readonly button = viewChildren<ElementRef>('myBtn');`,
},
{
id: 'viewChildren with class type locator and nullable',
before: `@ViewChildren(MyButton) button?: QueryList<MyButton>;`,
after: `readonly button = viewChildren(MyButton);`,
},
{
id: 'viewChildren with class type locator and exclamation mark',
before: `@ViewChildren(MyButton) button!: QueryList<MyButton>;`,
after: `readonly button = viewChildren(MyButton);`,
},
{
id: 'viewChild with string locator and read option, nullable shorthand',
before: `@ViewChildren('myBtn', {read: ElementRef}) buttonEl?: QueryList<ElementRef>;`,
after: `readonly buttonEl = viewChildren('myBtn', { read: ElementRef });`,
},
{
id: 'viewChildren with string locator and read option, required',
before: `@ViewChildren('myBtn', {read: ElementRef}) buttonEl!: QueryList<ElementRef>;`,
after: `readonly buttonEl = viewChildren('myBtn', { read: ElementRef });`,
},
{
id: 'viewChildren with query list as initializer value',
before: `@ViewChildren('myBtn') buttonEl = new QueryList<ElementRef>()`,
after: `readonly buttonEl = viewChildren<ElementRef>('myBtn');`,
},
{
id: 'viewChildren with query list as initializer value, and descendants option',
before: `@ViewChildren('myBtn', {descendants: false}) buttonEl = new QueryList<ElementRef>()`,
after: `readonly buttonEl = viewChildren<ElementRef>('myBtn', { descendants: false });`,
},
{
id: 'viewChildren with query list as initializer value, and descendants option but same as default',
before: `@ViewChildren('myBtn', {descendants: true}) buttonEl = new QueryList<ElementRef>()`,
after: `readonly buttonEl = viewChildren<ElementRef>('myBtn');`,
},
// ContentChildren
{
id: 'contentChildren with string locator and nullable',
before: `@ContentChildren('myBtn') button?: QueryList<ElementRef>;`,
after: `readonly button = contentChildren<ElementRef>('myBtn');`,
},
{
id: 'contentChildren with class type locator and nullable',
before: `@ContentChildren(MyButton) button?: QueryList<MyButton>;`,
after: `readonly button = contentChildren(MyButton);`,
},
{
id: 'contentChildren with class type locator and exclamation mark',
before: `@ContentChildren(MyButton) button!: QueryList<MyButton>;`,
after: `readonly button = contentChildren(MyButton);`,
},
{
id: 'contentChildren with string locator and read option, nullable shorthand',
before: `@ContentChildren('myBtn', {read: ElementRef}) buttonEl?: QueryList<ElementRef>;`,
after: `readonly buttonEl = contentChildren('myBtn', { read: ElementRef });`,
},
{
id: 'contentChildren with string locator and read option, required',
before: `@ContentChildren('myBtn', {read: ElementRef}) buttonEl!: QueryList<ElementRef>;`,
after: `readonly buttonEl = contentChildren('myBtn', { read: ElementRef });`,
},
{
id: 'contentChildren with query list as initializer value',
before: `@ContentChildren('myBtn') buttonEl = new QueryList<ElementRef>()`,
after: `readonly buttonEl = contentChildren<ElementRef>('myBtn');`,
},
{
id: 'contentChildren with query list as initializer value, and descendants option',
before: `@ContentChildren('myBtn', {descendants: true}) buttonEl = new QueryList<ElementRef>()`,
after: `readonly buttonEl = contentChildren<ElementRef>('myBtn', { descendants: true });`,
},
{
id: 'contentChildren with query list as initializer value, and descendants option but same as default',
before: `@ContentChildren('myBtn', {descendants: false}) buttonEl = new QueryList<ElementRef>()`,
after: `readonly buttonEl = contentChildren<ElementRef>('myBtn');`,
},
];
describe('signal queries migration', () => {
@@ -57,19 +181,24 @@ describe('signal queries migration', () => {
initMockFileSystem('Native');
});
describe('test cases', () => {
for (const c of testCases) {
describe('declaration test cases', () => {
for (const c of declarationTestCases) {
(c.focus ? fit : it)(c.id, async () => {
const fs = await runTsurgeMigration(new SignalQueriesMigration(), [
{
name: absoluteFrom('/app.component.ts'),
isProgramRootFile: true,
contents: populateTestCaseComponent(c.before),
contents: populateDeclarationTestCaseComponent(c.before),
},
]);
const actual = fs.readFile(absoluteFrom('/app.component.ts'));
const expected = populateTestCaseComponent(c.after);
let actual = fs.readFile(absoluteFrom('/app.component.ts'));
let expected = populateDeclarationTestCaseComponent(c.after);
// Cut off the string before the class declaration.
// The import diff is irrelevant here for now.
actual = actual.substring(actual.indexOf('@Directive'));
expected = expected.substring(expected.indexOf('@Directive'));
if (actual !== expected) {
expect(diffText(expected, actual)).toBe('');
@@ -102,9 +231,59 @@ describe('signal queries migration', () => {
expect(actual).not.toContain(`viewChild`);
expect(actual).toContain(`@ViewChild('labelRef') label?: ElementRef;`);
});
it('should update imports when migrating', async () => {
const fs = await runTsurgeMigration(new SignalQueriesMigration(), [
{
name: absoluteFrom('/app.component.ts'),
isProgramRootFile: true,
contents: `
import {ViewChild, ElementRef, Directive} from '@angular/core';
@Directive()
class MyComp {
@ViewChild('labelRef') label?: ElementRef;
}
`,
},
]);
const actual = fs.readFile(absoluteFrom('/app.component.ts'));
expect(actual).toContain(`import { ElementRef, Directive, viewChild } from '@angular/core';`);
expect(actual).toContain(`label = viewChild<ElementRef>('labelRef')`);
});
it('should not remove imports when partially migrating', async () => {
const fs = await runTsurgeMigration(new SignalQueriesMigration(), [
{
name: absoluteFrom('/app.component.ts'),
isProgramRootFile: true,
contents: `
import {ViewChild, ElementRef, Directive} from '@angular/core';
@Directive()
class MyComp {
@ViewChild('labelRef') label?: ElementRef;
@ViewChild('labelRef2') label2?: ElementRef;
click() {
this.label2 = undefined;
}
}
`,
},
]);
const actual = fs.readFile(absoluteFrom('/app.component.ts'));
expect(actual).toContain(
`import { ViewChild, ElementRef, Directive, viewChild } from '@angular/core';`,
);
expect(actual).toContain(`label = viewChild<ElementRef>('labelRef')`);
expect(actual).toContain(`@ViewChild('labelRef2') label2?: ElementRef;`);
});
});
function populateTestCaseComponent(declaration: string): string {
function populateDeclarationTestCaseComponent(declaration: string): string {
return `
import {
ViewChild,
packages/core/schematics/migrations/signal-queries-migration/migration.ts
+42 -1
View File
@@ -24,6 +24,9 @@ import {unwrapParent} from '../signal-migration/src/utils/unwrap_parent';
import {writeBinaryOperators} from '../signal-migration/src/utils/write_operators';
import {computeReplacementsToMigrateQuery} from './convert_query_property';
import {ImportManager, PartialEvaluator} from '../../../../compiler-cli/private/migrations';
import {applyImportManagerChanges} from '../../utils/tsurge/helpers/apply_import_manager';
import {QueryFunctionName} from '../../../../compiler-cli/src/ngtsc/annotations';
import {queryFunctionNameToDecorator} from './query_api_names';
export interface CompilationUnitData {
knownQueryFields: Record<ClassPropertyID, true>;
@@ -112,6 +115,9 @@ export class SignalQueriesMigration extends TsurgeComplexMigration<
const replacements: Replacement[] = [];
const importManager = new ImportManager();
const filesWithMigratedQueries = new Map<ts.SourceFile, Set<QueryFunctionName>>();
const filesWithIncompleteMigration = new Map<ts.SourceFile, Set<QueryFunctionName>>();
const isMigratedQuery = (id: ClassPropertyID) =>
globalMetadata.knownQueryFields[id] !== undefined &&
globalMetadata.problematicQueries[id] === undefined;
@@ -124,7 +130,13 @@ export class SignalQueriesMigration extends TsurgeComplexMigration<
evaluator,
projectDirAbsPath,
);
if (extractedQuery !== null && isMigratedQuery(extractedQuery.id)) {
if (extractedQuery !== null) {
if (!isMigratedQuery(extractedQuery.id)) {
updateFileState(filesWithIncompleteMigration, node, extractedQuery.kind);
return;
}
updateFileState(filesWithMigratedQueries, node, extractedQuery.kind);
replacements.push(
...computeReplacementsToMigrateQuery(
node as ts.PropertyDeclaration,
@@ -132,6 +144,7 @@ export class SignalQueriesMigration extends TsurgeComplexMigration<
importManager,
),
);
return;
}
// Migrate references to queries, if those are migrated too.
@@ -153,6 +166,34 @@ export class SignalQueriesMigration extends TsurgeComplexMigration<
ts.forEachChild(sf, visitor);
}
// Remove imports if possible.
for (const [file, types] of filesWithMigratedQueries) {
for (const type of types) {
if (!filesWithIncompleteMigration.get(file)?.has(type)) {
importManager.removeImport(file, queryFunctionNameToDecorator(type), '@angular/core');
}
}
}
applyImportManagerChanges(importManager, replacements, sourceFiles);
return replacements;
}
}
/**
* Updates the given map to capture the given query type.
* The map may track migrated queries in a file, or query types
* that couldn't be migrated.
*/
function updateFileState(
stateMap: Map<ts.SourceFile, Set<string>>,
node: ts.Node,
queryType: QueryFunctionName,
): void {
const file = node.getSourceFile();
if (!stateMap.has(file)) {
stateMap.set(file, new Set());
}
stateMap.get(file)!.add(queryType);
}
packages/core/schematics/migrations/signal-queries-migration/query_api_names.ts
+23
View File
@@ -0,0 +1,23 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import {QueryFunctionName} from '../../../../compiler-cli/src/ngtsc/annotations';
/** Converts an initializer query API name to its decorator-equivalent. */
export function queryFunctionNameToDecorator(name: QueryFunctionName): string {
if (name === 'viewChild') {
return 'ViewChild';
} else if (name === 'viewChildren') {
return 'ViewChildren';
} else if (name === 'contentChild') {
return 'ContentChild';
} else if (name === 'contentChildren') {
return 'ContentChildren';
}
throw new Error(`Unexpected query function name: ${name}`);
}
packages/core/schematics/migrations/signal-queries-migration/query_list_type.ts
+34
View File
@@ -0,0 +1,34 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import ts from 'typescript';
/**
* Extracts the type `T` of expressions referencing `QueryList<T>`.
*/
export function extractQueryListType(node: ts.TypeNode | ts.Expression): ts.TypeNode | undefined {
// Initializer variant of `new QueryList<T>()`.
if (
ts.isNewExpression(node) &&
ts.isIdentifier(node.expression) &&
node.expression.text === 'QueryList'
) {
return node.typeArguments?.[0];
}
// Type variant of `: QueryList<T>`.
if (
ts.isTypeReferenceNode(node) &&
ts.isIdentifier(node.typeName) &&
node.typeName.text === 'QueryList'
) {
return node.typeArguments?.[0];
}
return undefined;
}
packages/core/schematics/utils/tsurge/BUILD.bazel
+1
View File
@@ -13,6 +13,7 @@ ts_library(
"//packages/compiler-cli/src/ngtsc/file_system",
"//packages/compiler-cli/src/ngtsc/file_system/testing",
"//packages/compiler-cli/src/ngtsc/shims",
"//packages/compiler-cli/src/ngtsc/translator",
"@npm//@types/diff",
"@npm//@types/node",
"@npm//chalk",
packages/core/schematics/utils/tsurge/helpers/apply_import_manager.ts
+79
View File
@@ -0,0 +1,79 @@
/**
* @license
* Copyright Google LLC All Rights Reserved.
*
* Use of this source code is governed by an MIT-style license that can be
* found in the LICENSE file at https://angular.io/license
*/
import ts from 'typescript';
import {ImportManager} from '../../../../../compiler-cli/src/ngtsc/translator';
import {
absoluteFrom,
absoluteFromSourceFile,
} from '../../../../../compiler-cli/src/ngtsc/file_system';
import {Replacement, TextUpdate} from '../replacement';
/**
* Applies import manager changes, and writes them as replacements the
* given result array.
*/
export function applyImportManagerChanges(
importManager: ImportManager,
replacements: Replacement[],
sourceFiles: readonly ts.SourceFile[],
) {
const {newImports, updatedImports, deletedImports} = importManager.finalize();
const printer = ts.createPrinter({});
const pathToFile = new Map<string, ts.SourceFile>(sourceFiles.map((s) => [s.fileName, s]));
// Capture new imports
newImports.forEach((newImports, fileName) => {
newImports.forEach((newImport) => {
const printedImport = printer.printNode(
ts.EmitHint.Unspecified,
newImport,
pathToFile.get(fileName)!,
);
replacements.push(
new Replacement(
absoluteFrom(fileName),
new TextUpdate({position: 0, end: 0, toInsert: `${printedImport}\n`}),
),
);
});
});
// Capture updated imports
for (const [oldBindings, newBindings] of updatedImports.entries()) {
const printedBindings = printer.printNode(
ts.EmitHint.Unspecified,
newBindings,
oldBindings.getSourceFile(),
);
replacements.push(
new Replacement(
absoluteFromSourceFile(oldBindings.getSourceFile()),
new TextUpdate({
position: oldBindings.getStart(),
end: oldBindings.getEnd(),
toInsert: printedBindings,
}),
),
);
}
// Update removed imports
for (const removedImport of deletedImports) {
replacements.push(
new Replacement(
absoluteFromSourceFile(removedImport.getSourceFile()),
new TextUpdate({
position: removedImport.getStart(),
end: removedImport.getEnd(),
toInsert: '',
}),
),
);
}
}
packages/core/schematics/utils/tsurge/testing/diff.ts
+1 -1
View File
@@ -25,7 +25,7 @@ export function diffText(expected: string, actual: string): string {
? chalk.green(valueForColor)
: part.removed
? chalk.red(valueForColor)
: part.value;
: chalk.reset(part.value);
result += text;
}