Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improving refactoring discoverability and UX #34930

Closed
mjbvz opened this issue Nov 5, 2019 · 9 comments
Closed

Improving refactoring discoverability and UX #34930

mjbvz opened this issue Nov 5, 2019 · 9 comments
Assignees
Labels
Committed The team has roadmapped this issue Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript

Comments

@mjbvz
Copy link
Contributor

mjbvz commented Nov 5, 2019

Problem

Consider the following code:

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

TypeScript currently only returns refactorings for extract function and extract constant if you exactly select the range of the given expression. To extract a + 20 * 10 to a constant for example, I have to fully select a + 20 * 10.

There are a few limitations with this approach:

  • It can be difficult to discover refactorings and understand exactly what expression I need to select.
  • I have to take extra time to make the correct selection.
  • There is no feedback on why a given refactoring may not be available.

On the VS Code side we'd like to improve the discoverability and general UX around refactorings, and believe we need some new TS server work to make this happen.

Ideas

This issue in intended to be high level discussion about improving refactorings. We can split out specific proposals into their own issues.

As background, VS Code has two ways that refactorings can be requested:

  • Automatically as the user makes selections in the document. These are shown under the lightbulb menu
  • Explicitly using a keybinding or command.

Here are some initial ideas about improving both cases

Be less strict about selection range for automatic refactorings

To example cases:

  • If I select 20 * 1 in the example, automatically expand the refactor target to the subexpression 20 * 10
  • If I select the a + 20 * 10; with the trailing semicolon, automatically shrink the refactor to just the target expression a + 20 * 10.

For basic cases like the above, it may make sense to always perform some basic massaging of the selection. This would mean that these refactorings would show in VS Code's automatic lightbulb

More aggressive expanding of selections for explicit refactorings

If I explicitly request a refactoring such as extract constant, we could always try to expand the selection to the minimal extractable expression and possibly show multiple extract constant refactorings if multiple parent expressions could all be extracted.

In the example code, if I simply have the cursor before a in the expression a + 20 * 10 (with no selection), when I try to trigger extract constant then we could show refactorings for: extract expression 'a + 20 * 10' to constant

Feedback on why an explicitly requested refactoring is not available

When I explicitly request refactorings on a selection that cannot be refactored, we should show a message about why the refactoring is not possible.

@mjbvz mjbvz self-assigned this Nov 5, 2019
@fatcerberus
Copy link

My first question is, for cases where there is no selection, how do you decide how large of a subexpression (if not the whole thing!) to take? For example say I trigger the refactor on an expression within a function call, do we take only that argument, or take the entire function call? It seems like this question isn’t always going to be straightforward and could get pretty hairy. And people will necessarily have different opinions on what the right thing to do is...

@mjbvz
Copy link
Contributor Author

mjbvz commented Nov 6, 2019

We don't have to be perfect. The idea is that the more fuzzy expansion would show additional potential refactorings, not prevent using refactorings as they work today.

The main risk is that we don't want to show a bunch of not very help refactorings everywhere all the time—i.e. have the VS Code lightbulb always show up—which I why I propose that we are less aggressive expanding the selection for implicit refactoring requests vs explicit ones

@greyepoxy
Copy link

A couple of thoughts,
Resharper/intellij does this today (at least the selection expansion). Even just having a cursor (no-selection) on an expression will enable the refactoring context menu for the whole expression (or whatever symbol the cursor is on). I know that was not part of the proposal due to making the lightbulb show up all the time, but you might re-consider.

I played a little with this when trying to write automated refactorings for TS https://github.com/greyepoxy/typescript-refactoring-plugin/blob/master/test/refactorings/tryGetTargetExpression.tests.ts#L63. There is definitely some corner cases but as long as your consistent it should be fine (although I cannot say I was super consistent in my linked example).

@RyanCavanaugh RyanCavanaugh added Committed The team has roadmapped this issue Suggestion An idea for TypeScript labels Nov 8, 2019
@RyanCavanaugh RyanCavanaugh added this to the Backlog milestone Nov 8, 2019
@mjbvz
Copy link
Contributor Author

mjbvz commented Nov 14, 2019

I have split out two concrete proposals based on the ideas brought up:

@minestarks
Copy link
Member

@jessetrinity to address the selection range issues described here.

@jessetrinity
Copy link
Contributor

jessetrinity commented Apr 10, 2020

There are some cases for which I think it makes sense to offer an extract refactor for a cursor location.

function func1(){
    function func2(a: number){
        return a;
    }  
}

is another case where we only offer the extract refactor for an exact selection

    function func2(a: number){
        return a;
    }  

Assuming we change this, we wouldn't want to offer a refactor if the cursor (no selection) is anywhere in the function body (due to too many refactors as already mentioned), but we may want to offer an implicit refactor if the cursor is, say, within the function keyword or the identifier func2 as I don't imagine the cursor spends a lot of time in those ranges once they are completed, and if it's there, you're probably planning on changing something.

If there is an actual selection, we can be more liberal and offer a refactor if the start position of the selection is anywhere in function func2 or function func2(a: number). Some of our other refactors already work in this way, see convertParamsToDestructuredObject.

If I request refactors using ctrl + . for a cursor location we should make a good attempt to find something since this is the easiest and most obvious way to find out if there is a potential refactor nearby for which I don't have the exactly correct selection.

@mjbvz
Copy link
Contributor Author

mjbvz commented Apr 10, 2020

@jessetrinity That behavior sounds good to me. I've also opened #35096 about providing TS with information about why refactorings were requested. If you are experimenting with the behavior, I can hook up VS Code to send along a refactor trigger reason

@jessetrinity
Copy link
Contributor

jessetrinity commented Apr 10, 2020

That would be handy. I think @PranavSenthilnathan is already working on adding the trigger reason on our end (does this require more than just adding a triggerReason to the protocol)?

I listed my thoughts on other cases in #37895. They seem pretty straightforward but I noticed there might be some performance concerns with offering too many refactors while going through them.

@jessetrinity
Copy link
Contributor

Closing as completed by #41975

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Committed The team has roadmapped this issue Rescheduled This issue was previously scheduled to an earlier milestone Suggestion An idea for TypeScript
Projects
None yet
Development

No branches or pull requests

7 participants