From 280e240f5f978aaeaec19aa7e5f02ee5f2c9bb2e Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Mon, 14 Oct 2024 12:37:07 +0000 Subject: [PATCH] refactor(migrations): improve safety of queries migration when dealing with union types (#58190) When dealing with union types of queries, we should be conservative in the default mode and skip migration. That is because `viewChild()` always includes `undefined`, but we cannot trivially emulate the behavior where it was initially set to `null` (as an alternative to `undefined`). Those queries can still be migrated via best effort mode. PR Close #58190 --- .../problematic_patterns/incompatibility.ts | 9 +++-- .../incompatibility_human.ts | 5 +++ .../migration.spec.ts | 39 +++++++++++++++++++ .../signal-queries-migration/migration.ts | 26 +++++++++++-- 4 files changed, 71 insertions(+), 8 deletions(-) diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility.ts b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility.ts index c8504d913f0..fa6b6f9399b 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility.ts @@ -25,10 +25,11 @@ export enum FieldIncompatibilityReason { SignalInput__RequiredButNoGoodExplicitTypeExtractable = 8, SignalInput__QuestionMarkButNoGoodExplicitTypeExtractable = 9, SignalQueries__QueryListProblematicFieldAccessed = 10, - WriteAssignment = 11, - Accessor = 12, - OutsideOfMigrationScope = 13, - SkippedViaConfigFilter = 14, + SignalQueries__IncompatibleMultiUnionType = 11, + WriteAssignment = 12, + Accessor = 13, + OutsideOfMigrationScope = 14, + SkippedViaConfigFilter = 15, } /** Field reasons that cannot be ignored. */ diff --git a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility_human.ts b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility_human.ts index 250dc55f8bf..2fa886b1b22 100644 --- a/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility_human.ts +++ b/packages/core/schematics/migrations/signal-migration/src/passes/problematic_patterns/incompatibility_human.ts @@ -75,6 +75,11 @@ export function getMessageForFieldIncompatibility( short: `There are references to this query that cannot be migrated automatically.`, extra: "For example, it's not possible to migrate `.changes` or `.dirty` trivially.", }; + case FieldIncompatibilityReason.SignalQueries__IncompatibleMultiUnionType: + return { + short: `Query type is too complex to automatically migrate.`, + extra: "The new query API doesn't allow us to migrate safely without breaking your app.", + }; case FieldIncompatibilityReason.SkippedViaConfigFilter: return { short: `This ${fieldName.single} is not part of the current migration scope.`, diff --git a/packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts b/packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts index 27face39517..a9b7ed3d1c5 100644 --- a/packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts +++ b/packages/core/schematics/migrations/signal-queries-migration/migration.spec.ts @@ -1303,6 +1303,45 @@ describe('signal queries migration', () => { ).not.toBeRejected(); }); + it('should properly deal with union types of single queries', async () => { + const {fs} = await runTsurgeMigration(new SignalQueriesMigration({}), [ + { + name: absoluteFrom('/app.component.ts'), + isProgramRootFile: true, + contents: dedent` + import {ViewChild, ElementRef, Component} from '@angular/core'; + + @Component({ + template: '', + }) + class MyComp { + @ViewChild(MyService) bla: MyService|undefined = undefined; + @ViewChild(MyService) bla2?: MyService; + @ViewChild(MyService) bla3: MyService|null = null; + @ViewChild(MyService) bla4: MyService|SomethingUnrelated = null!; + @ViewChild(MyService) bla5!: MyService|SomethingUnrelated; + } + `, + }, + ]); + + const actual = fs.readFile(absoluteFrom('/app.component.ts')); + expect(actual).toMatchWithDiff(` + import {ViewChild, ElementRef, Component, viewChild} from '@angular/core'; + + @Component({ + template: '', + }) + class MyComp { + readonly bla = viewChild(MyService); + readonly bla2 = viewChild(MyService); + @ViewChild(MyService) bla3: MyService|null = null; + @ViewChild(MyService) bla4: MyService|SomethingUnrelated = null!; + @ViewChild(MyService) bla5!: MyService|SomethingUnrelated; + } + `); + }); + describe('--best-effort-mode', () => { it('should be possible to forcibly migrate even with a detected `.changes` access', async () => { const {fs} = await runTsurgeMigration(new SignalQueriesMigration({bestEffortMode: true}), [ diff --git a/packages/core/schematics/migrations/signal-queries-migration/migration.ts b/packages/core/schematics/migrations/signal-queries-migration/migration.ts index 9a136b458f5..c6847dc2fa8 100644 --- a/packages/core/schematics/migrations/signal-queries-migration/migration.ts +++ b/packages/core/schematics/migrations/signal-queries-migration/migration.ts @@ -123,11 +123,12 @@ export class SignalQueriesMigration extends TsurgeComplexMigration< const findQueryDefinitionsVisitor = (node: ts.Node) => { const extractedQuery = extractSourceQueryDefinition(node, reflector, evaluator, info); if (extractedQuery !== null) { + const queryNode = extractedQuery.node; const descriptor = { key: extractedQuery.id, - node: extractedQuery.node, + node: queryNode, }; - const containingFile = projectFile(descriptor.node.getSourceFile(), info); + const containingFile = projectFile(queryNode.getSourceFile(), info); // If we have a config filter function, use it here for later // perf-boosted reference lookups. Useful in non-batch mode. @@ -135,7 +136,7 @@ export class SignalQueriesMigration extends TsurgeComplexMigration< this.config.shouldMigrateQuery === undefined || this.config.shouldMigrateQuery(descriptor, containingFile) ) { - classesWithFilteredQueries.add(extractedQuery.node.parent); + classesWithFilteredQueries.add(queryNode.parent); filteredQueriesForCompilationUnit.set(extractedQuery.id, { fieldName: extractedQuery.queryInfo.propertyName, }); @@ -146,13 +147,30 @@ export class SignalQueriesMigration extends TsurgeComplexMigration< isMulti: extractedQuery.queryInfo.first === false, }; - if (ts.isAccessor(extractedQuery.node)) { + if (ts.isAccessor(queryNode)) { markFieldIncompatibleInMetadata( res.potentialProblematicQueries, extractedQuery.id, FieldIncompatibilityReason.Accessor, ); } + + // Detect queries with union types that are uncommon to be + // automatically migrate-able. E.g. `refs: ElementRef|null`, + // or `ElementRef|SomeOtherType`. + if ( + queryNode.type !== undefined && + ts.isUnionTypeNode(queryNode.type) && + // Either too large union, or doesn't match `T|undefined`. + (queryNode.type.types.length > 2 || + !queryNode.type.types.some((t) => t.kind === ts.SyntaxKind.UndefinedKeyword)) + ) { + markFieldIncompatibleInMetadata( + res.potentialProblematicQueries, + extractedQuery.id, + FieldIncompatibilityReason.SignalQueries__IncompatibleMultiUnionType, + ); + } } };