Cases for refactor discoverability

[jessetrinity]

Refactoring cases for which we can improve discoverability

This is the list of our refactors TSServer offers and the proposed changes to make them more discoverable. See #34930 for the broader discussion.

Here I have listed the current behavior as well as proposed changes to make the refactors more discoverable. Some of these ideas may offer refactors too frequently/infrequently, so more input is desired. The explicit refactoring request cases mentioned depend on #35096.

This issue does not list any code fixes.

addOrRemoveBracesToArrowFunction

Case - remove braces:

const a = (n: number) => {
    return n;
}

Current:

• Refactor offered for cursor or selection start in range (n: number). selection end can be anywhere else.

Proposed:

• Explicit request offers refactor for cursor or selection anywhere in function including body.
• Implict refactor seems OK as is.

convertExport

Case - Convert default export to named export:

export default function f();

Case - Convert named export to default:

export function f();

Current:

• Refactor offered only when entire range is selected.

Proposed:

• Explicit request offers refactor for cursor or selection anywhere in range.
• Implicit refactor triggers for cursor or selection anywhere in ranges f() or default.
• Note this refactor only applies when the only statement in the function is a return statement, so we aren't in danger of offering up this refactor too much. We may even be fine offering it as often as in the explicit case.

convertImport

Case - Convert namespace import to named imports:

import * as a from "./a";

Case - Convert named import to namespace import:

import { f } from "./a";

Current:

• Refactor offered only when entire range is selected.

Proposed:

• Explicit request offers refactor for cursor or selection start anywhere in range. Implicit refactor triggers anywhere in * as a or { f }.

extractSymbol

Case - extract const

function foo(a: any) {
    return a + 20 * 10;
}

Current:

• No refactor offered for cursor anywhere within statement.
• Refactor offered for selection if selection start and end are within the same Literal. For example, selecting 2 offers to extract 20. This is not the case for other valid extraction candidates such as Identifiers or functions.
• No refactor offered for selections if selection start and end are in the LHS and RHS of the same binary expression, but do not span it. For example, if 20 + 1 is selected, no refactor is offered.
• See Improving refactoring discoverability and UX #34930.

Proposed:

• Treat selections as though they span the nodes in which the ends terminate.
• Treat an explicit refactor request for a cursor as though the largest node starting at that location is selected.

Case - extract method

function foo(){
    function bar(){
        return 0;
    }
}

Current

• Refactor offered only if entire inner function is selected.

Proposed:

• Implicitly offer extraction for functions for a cursor in the function name or function keyword (or both).
• Offer refactor for selections if the start posisiton is anywhere in function bar.

extractType

Case - extract to type alias

var y: string = '';

Current:

• Must have entire type string selected.

Proposed:

• Implicitly treat partial selection of string as the selecting whole type.
• Explicit refactor requests should offer the refactor if the cursor is in string

Case - extract to interface

var x: { a?: number, b?: string } = { };

Current:

• Must have entire type literal { a?: number, b?: string } selected.

Proposed:

• Offer refactor if cursor is at either brace location.

generateGetAccessorAndSetAccessor

Case - Generate 'get' and 'set' accessors

class A {
    public convertMe: string;
}

Current:

• Refactor offered when selection start or end is in range convertMe.

Proposed:

• Offer refactor on explicit request for cursor or selection start anywhere in the property declaration.

Refactors that are probably okay

Included for completeness. These cases are probably fine as is.

convertParamsToDestructuredObject

function f(a: number, b: number) {
    return a + b;
}

Current:

• Refactor offered for cursor or selection start in range function f(a: number, b: number. Selection end can be anywhere else.

convertStringOrTemplateLiteral

const a = "as above, ";
const b = a + "so below";

Current:

• Refactor offered for cursor or selection start in range a + "so below. Selection end can be anywhere else.

moveToNewFile

console.log("goodbye");

Current:

• Must have entire statement selected.
• Since so many things can be moved to a new file this refactor is already very common and we probably don't want to offer it up too much.

[mjbvz]

This list and the expected behavior look good to me. Thanks for putting it together!

[jomi-se]

Hello 👋

I was curious about this feature because tide only offers a single tide-refactor function.
It doesn't have much documentation on what it does and I only recently realized it is calling tsserver getApplicableRefactorings internally.

I tried looking around the documentation here and only through these issues did I figure out how this stuff works on the tsserver side (One has to select a chunk of text to be sent to tsserver to get the refactoring candidates available in that position)

Is there a list of the available refactorings somewhere or some documentation on them? I'm interested in improving the UX for this stuff on the tide side but am having trouble figuring out how it all works on the tsserver side. It'd be cool to have a list of the refactoring capabilities supported by tsserver.

Btw, thanks a lot for the work on all of this. It seems almost magical 😍

(sorry if this isn't the right place to ask this but it seemed related to "refactor discoverability 🙈 )

[jessetrinity]

Refactors live here:
https://github.com/microsoft/TypeScript/tree/master/src/services/refactors

There has not been great documentation for available refactors and code fixes in the past, but we're working on changing that. I think the best we have at the moment is https://github.com/microsoft/JSTSdocs/blob/master/articles/editor/refactoring.md.

To get an idea of how they work, I found it helpful to set a breakpoint in the function that gets the available refactors for a given span, for example getImportToConvert in convertImport.ts, and note that it will be called twice over the lifetime of a refactor operation.

An editor should first send getApplicableRefactors to get a list of available refactors by checking each refactor to see which are applicable to the requested span. When a user selects one of the offered refactors, the editor would then want to send getEditsForRefactor with the selected refactor. tsserver will then match the requested refactor with one from the applicable refactors list and perform the refactor.

[jomi-se]

Thank you for the quick reply @jessetrinity ! That is useful 👍

I was wondering because as it currently stands the UX of these refactorings seem a bit hard to discover naturally while coding. Right now, you just select a bunch of code randomly and see what comes up as a reply from getApplicableRefactors (at least on the emacs world, don't know how it is used on vscode). Maybe just pointing users to the doc could be good enough.

[jessetrinity]

Yeah, having to select the exact span does make them hard to discover. VS and VSCode have an icon that pops up when a span is selected to indicate that a refactor is available. The effort associated with this issue is returning the refactors if the span is not exactly selected, or if the cursor is simply within a valid span when getApplicableRefactors is sent. Adding more docs is a good idea too, thanks for that input!

I'm only vaguely familiar with how this looks in emacs. Is requesting available refactors at a cursor location something that would fit into the workflow for emacs users (is there a default key command to send the request)?

The idea is that, in VSCode for example, using the refactor shortcut ctrl + . when the cursor is at /**/ in the following would return the extractSymbol refactor for 100 + 200.

function foo(){
    return 1/**/00 + 200;
}

[jessetrinity]

Closing following #38378 which added the refactor trigger reason and expanding the spans for some refactors. If we want to tweak any of the spans further we can open items for those refactors.