From ee14b55d5c42bfa25ff03a2207bbbee96dc42a4a Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 18 Oct 2024 13:28:15 +0000 Subject: [PATCH 1/2] feat(language-service): support migrating full classes to signal queries Adds a similar code action as for inputs, where users can migrate full classes to signal queries. --- .../apply_query_refactoring.ts | 2 +- .../full_class_query_refactoring.ts | 117 ++++++++ .../src/refactorings/refactoring.ts | 6 + .../signal_queries_refactoring_action_spec.ts | 265 +++++++++++++++++- 4 files changed, 384 insertions(+), 6 deletions(-) create mode 100644 packages/language-service/src/refactorings/convert_to_signal_queries/full_class_query_refactoring.ts diff --git a/packages/language-service/src/refactorings/convert_to_signal_queries/apply_query_refactoring.ts b/packages/language-service/src/refactorings/convert_to_signal_queries/apply_query_refactoring.ts index 597368e69e0a..3eeae93da4f3 100644 --- a/packages/language-service/src/refactorings/convert_to_signal_queries/apply_query_refactoring.ts +++ b/packages/language-service/src/refactorings/convert_to_signal_queries/apply_query_refactoring.ts @@ -106,7 +106,7 @@ export async function applySignalQueriesRefactoring( } else if (incompatibilityMessages.size > 0) { const queryPlural = incompatibilityMessages.size === 1 ? 'query' : `queries`; message = `${incompatibilityMessages.size} ${queryPlural} could not be migrated.\n`; - message += `For more details, click on the skipped inputs and try to migrate individually.\n`; + message += `For more details, click on the skipped queries and try to migrate individually.\n`; } // Only suggest the "force ignoring" option if there are actually diff --git a/packages/language-service/src/refactorings/convert_to_signal_queries/full_class_query_refactoring.ts b/packages/language-service/src/refactorings/convert_to_signal_queries/full_class_query_refactoring.ts new file mode 100644 index 000000000000..7eea01ba2efe --- /dev/null +++ b/packages/language-service/src/refactorings/convert_to_signal_queries/full_class_query_refactoring.ts @@ -0,0 +1,117 @@ +/** + * @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.dev/license + */ + +import {CompilerOptions} from '@angular/compiler-cli'; +import {NgCompiler} from '@angular/compiler-cli/src/ngtsc/core'; +import {MigrationConfig} from '@angular/core/schematics/migrations/signal-migration/src'; +import {ApplyRefactoringProgressFn, ApplyRefactoringResult} from '@angular/language-service/api'; +import ts from 'typescript'; +import {isTypeScriptFile} from '../../utils'; +import {findTightestNode, getParentClassDeclaration} from '../../utils/ts_utils'; +import type {ActiveRefactoring} from '../refactoring'; +import {isDecoratorQueryClassField, isDirectiveOrComponentWithQueries} from './decorators'; +import {applySignalQueriesRefactoring} from './apply_query_refactoring'; + +/** + * Base language service refactoring action that can convert decorator + * queries of a full class to signal queries. + * + * The user can click on an class with decorator queries and ask for all the queries + * to be migrated. All references, imports and the declaration are updated automatically. + */ +abstract class BaseConvertFullClassToSignalQueriesRefactoring implements ActiveRefactoring { + abstract config: MigrationConfig; + + constructor(private project: ts.server.Project) {} + + static isApplicable( + compiler: NgCompiler, + fileName: string, + positionOrRange: number | ts.TextRange, + ): boolean { + if (!isTypeScriptFile(fileName)) { + return false; + } + + const sf = compiler.getCurrentProgram().getSourceFile(fileName); + if (sf === undefined) { + return false; + } + + const start = typeof positionOrRange === 'number' ? positionOrRange : positionOrRange.pos; + const node = findTightestNode(sf, start); + if (node === undefined) { + return false; + } + + const classDecl = getParentClassDeclaration(node); + if (classDecl === undefined) { + return false; + } + const {reflector} = compiler['ensureAnalyzed'](); + if (!isDirectiveOrComponentWithQueries(classDecl, reflector)) { + return false; + } + + const parentClassElement = ts.findAncestor(node, (n) => ts.isClassElement(n) || ts.isBlock(n)); + if (parentClassElement === undefined) { + return true; + } + // If we are inside a body of e.g. an accessor, this action should not show up. + if (ts.isBlock(parentClassElement)) { + return false; + } + return isDecoratorQueryClassField(parentClassElement, reflector); + } + + async computeEditsForFix( + compiler: NgCompiler, + compilerOptions: CompilerOptions, + fileName: string, + positionOrRange: number | ts.TextRange, + reportProgress: ApplyRefactoringProgressFn, + ): Promise { + const sf = compiler.getCurrentProgram().getSourceFile(fileName); + if (sf === undefined) { + return {edits: []}; + } + + const start = typeof positionOrRange === 'number' ? positionOrRange : positionOrRange.pos; + const node = findTightestNode(sf, start); + if (node === undefined) { + return {edits: []}; + } + + const containingClass = getParentClassDeclaration(node); + if (containingClass === null) { + return {edits: [], errorMessage: 'Could not find a class for the refactoring.'}; + } + + return await applySignalQueriesRefactoring( + compiler, + compilerOptions, + this.config, + this.project, + reportProgress, + (queryID) => queryID.node.parent === containingClass, + /** allowPartialMigration */ true, + ); + } +} + +export class ConvertFullClassToSignalQueriesRefactoring extends BaseConvertFullClassToSignalQueriesRefactoring { + static id = 'convert-full-class-to-signal-queries-safe-mode'; + static description = 'Full class: Convert all decorator queries to signal queries (safe)'; + override config: MigrationConfig = {}; +} +export class ConvertFullClassToSignalQueriesBestEffortRefactoring extends BaseConvertFullClassToSignalQueriesRefactoring { + static id = 'convert-full-class-to-signal-queries-best-effort-mode'; + static description = + 'Full class: Convert all decorator queries to signal queries (forcibly, ignoring errors)'; + override config: MigrationConfig = {bestEffortMode: true}; +} diff --git a/packages/language-service/src/refactorings/refactoring.ts b/packages/language-service/src/refactorings/refactoring.ts index 839571ed3f4f..e8df6d2c87c5 100644 --- a/packages/language-service/src/refactorings/refactoring.ts +++ b/packages/language-service/src/refactorings/refactoring.ts @@ -22,6 +22,10 @@ import { ConvertFieldToSignalQueryBestEffortRefactoring, ConvertFieldToSignalQueryRefactoring, } from './convert_to_signal_queries/individual_query_refactoring'; +import { + ConvertFullClassToSignalQueriesBestEffortRefactoring, + ConvertFullClassToSignalQueriesRefactoring, +} from './convert_to_signal_queries/full_class_query_refactoring'; /** * Interface exposing static metadata for a {@link Refactoring}, @@ -80,4 +84,6 @@ export const allRefactorings: Refactoring[] = [ // Queries migration ConvertFieldToSignalQueryRefactoring, ConvertFieldToSignalQueryBestEffortRefactoring, + ConvertFullClassToSignalQueriesRefactoring, + ConvertFullClassToSignalQueriesBestEffortRefactoring, ]; diff --git a/packages/language-service/test/signal_queries_refactoring_action_spec.ts b/packages/language-service/test/signal_queries_refactoring_action_spec.ts index ae7cba8fbd82..9edcbeef974b 100644 --- a/packages/language-service/test/signal_queries_refactoring_action_spec.ts +++ b/packages/language-service/test/signal_queries_refactoring_action_spec.ts @@ -35,9 +35,11 @@ describe('Signal queries refactoring action', () => { appFile.moveCursorToText('re¦f!: ElementRef'); const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); - expect(refactorings.length).toBe(2); + expect(refactorings.length).toBe(4); expect(refactorings[0].name).toBe('convert-field-to-signal-query-safe-mode'); expect(refactorings[1].name).toBe('convert-field-to-signal-query-best-effort-mode'); + expect(refactorings[2].name).toBe('convert-full-class-to-signal-queries-safe-mode'); + expect(refactorings[3].name).toBe('convert-full-class-to-signal-queries-best-effort-mode'); }); it('should not support refactoring a non-Angular property', () => { @@ -97,9 +99,11 @@ describe('Signal queries refactoring action', () => { appFile.moveCursorToText('re¦f!: ElementRef'); const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); - expect(refactorings.length).toBe(2); + expect(refactorings.length).toBe(4); expect(refactorings[0].name).toBe('convert-field-to-signal-query-safe-mode'); expect(refactorings[1].name).toBe('convert-field-to-signal-query-best-effort-mode'); + expect(refactorings[2].name).toBe('convert-full-class-to-signal-queries-safe-mode'); + expect(refactorings[3].name).toBe('convert-full-class-to-signal-queries-best-effort-mode'); const edits = await project.applyRefactoring( 'app.ts', @@ -145,9 +149,11 @@ describe('Signal queries refactoring action', () => { appFile.moveCursorToText('bl¦a: ElementRef'); const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); - expect(refactorings.length).toBe(2); + expect(refactorings.length).toBe(4); expect(refactorings[0].name).toBe('convert-field-to-signal-query-safe-mode'); expect(refactorings[1].name).toBe('convert-field-to-signal-query-best-effort-mode'); + expect(refactorings[2].name).toBe('convert-full-class-to-signal-queries-safe-mode'); + expect(refactorings[3].name).toBe('convert-full-class-to-signal-queries-best-effort-mode'); const edits = await project.applyRefactoring( 'app.ts', @@ -179,9 +185,11 @@ describe('Signal queries refactoring action', () => { appFile.moveCursorToText('set bl¦a('); const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); - expect(refactorings.length).toBe(2); + expect(refactorings.length).toBe(4); expect(refactorings[0].name).toBe('convert-field-to-signal-query-safe-mode'); expect(refactorings[1].name).toBe('convert-field-to-signal-query-best-effort-mode'); + expect(refactorings[2].name).toBe('convert-full-class-to-signal-queries-safe-mode'); + expect(refactorings[3].name).toBe('convert-full-class-to-signal-queries-best-effort-mode'); const edits = await project.applyRefactoring( 'app.ts', @@ -243,9 +251,11 @@ describe('Signal queries refactoring action', () => { appFile.moveCursorToText('re¦f?: ElementRef'); const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); - expect(refactorings.length).toBe(2); + expect(refactorings.length).toBe(4); expect(refactorings[0].name).toBe('convert-field-to-signal-query-safe-mode'); expect(refactorings[1].name).toBe('convert-field-to-signal-query-best-effort-mode'); + expect(refactorings[2].name).toBe('convert-full-class-to-signal-queries-safe-mode'); + expect(refactorings[3].name).toBe('convert-full-class-to-signal-queries-best-effort-mode'); const edits = await project.applyRefactoring( 'app.ts', @@ -269,4 +279,249 @@ describe('Signal queries refactoring action', () => { }, ]); }); + + describe('full class', () => { + it('should support refactoring multiple query properties', () => { + const files = { + 'app.ts': ` + import {Directive, ViewChild, ContentChild, ElementRef} from '@angular/core'; + + @Directive({}) + export class AppComponent { + @ViewChild('refA') refA!: ElementRef; + @ContentChild('refB') refB?: ElementRef; + } + `, + }; + + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + const appFile = project.openFile('app.ts'); + appFile.moveCursorToText('App¦Component'); + const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); + + expect(refactorings.length).toBe(2); + expect(refactorings[0].name).toBe('convert-full-class-to-signal-queries-safe-mode'); + expect(refactorings[1].name).toBe('convert-full-class-to-signal-queries-best-effort-mode'); + }); + + it('should not suggest options when inside an accessor query body', async () => { + const files = { + 'app.ts': ` + import {Directive, ViewChild, ElementRef} from '@angular/core'; + + @Directive({}) + export class AppComponent { + @ViewChild('ref') + set bla(value: ElementRef) { + // hello + }; + } + `, + }; + + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + const appFile = project.openFile('app.ts'); + appFile.moveCursorToText('hell¦o'); + + const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); + expect(refactorings.length).toBe(0); + }); + + it('should generate edits for migrating multiple query properties', async () => { + const files = { + 'app.ts': ` + import {Directive, ViewChild, ContentChild, ElementRef} from '@angular/core'; + + @Directive({}) + export class AppComponent { + @ViewChild('refA') refA!: ElementRef; + @ContentChild('refB') refB?: ElementRef; + } + `, + }; + + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + const appFile = project.openFile('app.ts'); + appFile.moveCursorToText('App¦Component'); + const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); + + expect(refactorings.length).toBe(2); + expect(refactorings[0].name).toBe('convert-full-class-to-signal-queries-safe-mode'); + expect(refactorings[1].name).toBe('convert-full-class-to-signal-queries-best-effort-mode'); + + const result = await project.applyRefactoring( + 'app.ts', + appFile.cursor, + refactorings[0].name, + () => {}, + ); + + expect(result).toBeDefined(); + expect(result?.errorMessage).toBe(undefined); + expect(result?.warningMessage).toBe(undefined); + expect(result?.edits).toEqual([ + { + fileName: '/test/app.ts', + textChanges: [ + // Query declarations. + { + newText: `readonly refA = viewChild.required('refA');`, + span: {start: 165, length: `@ViewChild('refA') refA!: ElementRef;`.length}, + }, + { + newText: `readonly refB = contentChild('refB');`, + span: {start: 215, length: `@ContentChild('refB') refB?: ElementRef;`.length}, + }, + // Import. + { + newText: '{Directive, ElementRef, viewChild, contentChild}', + span: {start: 18, length: 48}, + }, + ], + }, + ]); + }); + + it('should generate edits for partially migrating multiple query properties', async () => { + const files = { + 'app.ts': ` + import {Directive, ViewChild, ContentChild, ElementRef} from '@angular/core'; + + @Directive({}) + export class AppComponent { + @ViewChild('refA') refA!: ElementRef; + @ContentChild('refB') refB?: ElementRef; + + click() { + this.refB = undefined; + } + } + `, + }; + + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + const appFile = project.openFile('app.ts'); + appFile.moveCursorToText('App¦Component'); + const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); + + expect(refactorings.length).toBe(2); + expect(refactorings[0].name).toBe('convert-full-class-to-signal-queries-safe-mode'); + expect(refactorings[1].name).toBe('convert-full-class-to-signal-queries-best-effort-mode'); + + const result = await project.applyRefactoring( + 'app.ts', + appFile.cursor, + refactorings[0].name, + () => {}, + ); + + expect(result).toBeDefined(); + expect(result?.warningMessage).toContain('1 query could not be migrated.'); + expect(result?.warningMessage).toContain( + 'click on the skipped queries and try to migrate individually.', + ); + expect(result?.warningMessage).toContain('action to forcibly convert.'); + expect(result?.errorMessage).toBe(undefined); + expect(result?.edits).toEqual([ + { + fileName: '/test/app.ts', + textChanges: [ + // Query declarations. + { + newText: `readonly refA = viewChild.required('refA');`, + span: {start: 165, length: `@ViewChild('refA') refA!: ElementRef;`.length}, + }, + // Import + { + newText: '{Directive, ContentChild, ElementRef, viewChild}', + span: {start: 18, length: 48}, + }, + ], + }, + ]); + }); + + it('should error when no queries could be migrated', async () => { + const files = { + 'app.ts': ` + import {Directive, ViewChild, ViewChildren, QueryList, ElementRef} from '@angular/core'; + + @Directive({}) + export class AppComponent { + @ViewChild('ref1') bla!: ElementRef; + @ViewChildren('refs') bla2!: QueryList; + + click() { + this.bla = undefined; + this.bla2.changes.subscribe(); + } + } + `, + }; + + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + const appFile = project.openFile('app.ts'); + appFile.moveCursorToText('App¦Component'); + const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); + + expect(refactorings.length).toBe(2); + expect(refactorings[0].name).toBe('convert-full-class-to-signal-queries-safe-mode'); + expect(refactorings[1].name).toBe('convert-full-class-to-signal-queries-best-effort-mode'); + + const result = await project.applyRefactoring( + 'app.ts', + appFile.cursor, + refactorings[0].name, + () => {}, + ); + + expect(result).toBeDefined(); + expect(result?.errorMessage).toContain('2 queries could not be migrated.'); + expect(result?.errorMessage).toContain( + 'click on the skipped queries and try to migrate individually.', + ); + expect(result?.errorMessage).toContain('action to forcibly convert.'); + expect(result?.warningMessage).toBe(undefined); + expect(result?.edits).toEqual([]); + }); + + it('should not suggest force mode when all queries are incompatible and non-ignorable', async () => { + const files = { + 'app.ts': ` + import {Directive, ViewChild} from '@angular/core'; + + @Directive({}) + export class AppComponent { + @ViewChild('ref1') set bla(v: string) {}; + @ViewChild('ref2') set bla2(v: string) {}; + } + `, + }; + + const project = createModuleAndProjectWithDeclarations(env, 'test', files); + const appFile = project.openFile('app.ts'); + appFile.moveCursorToText('App¦Component'); + const refactorings = project.getRefactoringsAtPosition('app.ts', appFile.cursor); + + expect(refactorings.length).toBe(2); + expect(refactorings[0].name).toBe('convert-full-class-to-signal-queries-safe-mode'); + expect(refactorings[1].name).toBe('convert-full-class-to-signal-queries-best-effort-mode'); + + const result = await project.applyRefactoring( + 'app.ts', + appFile.cursor, + refactorings[0].name, + () => {}, + ); + + expect(result).toBeDefined(); + expect(result?.errorMessage).toContain('2 queries could not be migrated.'); + expect(result?.errorMessage).toContain( + 'click on the skipped queries and try to migrate individually.', + ); + expect(result?.errorMessage).not.toContain('action to forcibly convert.'); + expect(result?.warningMessage).toBe(undefined); + expect(result?.edits).toEqual([]); + }); + }); }); From d07117ed5eb6997a5eed2c17e8e639fd452a472b Mon Sep 17 00:00:00 2001 From: Paul Gschwendtner Date: Fri, 18 Oct 2024 13:30:49 +0000 Subject: [PATCH 2/2] refactor(language-service): improve name of input full class refactoring action Prefixing the full class refactoring actions with `Full class` makes them less confusing along with the individual field actions. --- .../convert_to_signal_input/full_class_input_refactoring.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/language-service/src/refactorings/convert_to_signal_input/full_class_input_refactoring.ts b/packages/language-service/src/refactorings/convert_to_signal_input/full_class_input_refactoring.ts index 1ea65be2e190..cd2d22ed0956 100644 --- a/packages/language-service/src/refactorings/convert_to_signal_input/full_class_input_refactoring.ts +++ b/packages/language-service/src/refactorings/convert_to_signal_input/full_class_input_refactoring.ts @@ -106,11 +106,12 @@ abstract class BaseConvertFullClassToSignalInputsRefactoring implements ActiveRe export class ConvertFullClassToSignalInputsRefactoring extends BaseConvertFullClassToSignalInputsRefactoring { static id = 'convert-full-class-to-signal-inputs-safe-mode'; - static description = "Convert all class @Input's to signal inputs (safe)"; + static description = "Full class: Convert all @Input's to signal inputs (safe)"; override config: MigrationConfig = {}; } export class ConvertFullClassToSignalInputsBestEffortRefactoring extends BaseConvertFullClassToSignalInputsRefactoring { static id = 'convert-full-class-to-signal-inputs-best-effort-mode'; - static description = "Convert all class @Input's to signal inputs (forcibly, ignoring errors)"; + static description = + "Full class: Convert all @Input's to signal inputs (forcibly, ignoring errors)"; override config: MigrationConfig = {bestEffortMode: true}; }