GeezerVison is showing github.com — Improving refactoring discoverability and UX · Issue #34930 · microsoft/TypeScript
Improving refactoring discoverability and UX #34930DescriptionProblem
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. Reacted by Justin, Jesse Trinity, ExE Boss, A. Matías Quezada and Ebuall Activityfatcerberus commented on Nov 5, 2019 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 commented on Nov 6, 2019 ContributorAuthor More actions 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 commented on Nov 8, 2019 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). mjbvz commented on Nov 14, 2019 ContributorAuthor More actions I have split out two concrete proposals based on the ideas brought up:
minestarks commented on Apr 8, 2020 jessetrinity commented on 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 commented on Apr 10, 2020 ContributorAuthor More actions Jesse Trinity (@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 commented on Apr 10, 2020 That would be handy. I think Pranav Senthilnathan (@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 commented on Jan 11, 2021 Closing as completed by #41975
MetadataMetadataDevelopmentNo branches or pull requestsIssue actions
|