Repository navigation
Suggestion: refactor to "move to another (existing) file" #29988
Description
Activity
Yes please. This is slowing me down trying to refactor an existing code base currently. Having to manually copy paste + edit imports means I don't do this when I should be.
Reacted by Ethan Resnick and KUse case: I work in a large Typescript codebase that does not enforce (e.g. via lint rules) anything about creating cyclic dependencies. We'd like to, but first we need to break the existing cycles! But over the years, there's one dependency cycle that has grown to include 500 files. Half of the work to break a cycle is really just moving definitions between files...so this feature would be super helpful!
I'm sure this is harder than to move to a new file since now you'd need to check for name clashes (both the thing being moved and the imports that get moved), merge imports when they already exist on the destination file, etc.
This looks kind of fun so I might take a look at it...some guidance would be appreciated though! Recommendations on approaches would be helpful. I'm sure there's a lot of other issues I didn't think of.
Reacted by Rishabh Rao and mdornseifIt would be nice to have this feature!
It should be similar to #23726 im guessing if someone wanted to try implementing this they could look at that PRRyan Cavanaugh (@RyanCavanaugh) did you still need more feedback for this?
Another use case:
I'm mostly OK with "Move to a new file", but sometimes I move a bunch of symbols together and after saving and making further changes, I realize I forgot to move one symbol into that file, and have to do that manually instead.Reacted by Henning Dieterichs and Neil de Carteret+1
IMO "Move to an existing file" is much more useful than merely creating a new file.
Reacted by Ben McLean, Simone De Cristofaro, Jason Williams, Jules Sam. Randolph, thunderkid, uglycoyote, Edsolater, 0xdef1cafe, jtwigg, Andy Bulka and 16 morePaging Andrew Branch (@andrewbranch) is this difficult to implement?
I'm not sure of the internals, but this would probably be tricky - to my knowledge there are no refactor/fixits which require users pick a file or provide some sort of input in order for the fixit to run. You might have to make changes to the TS types and have editors accept the new format ahead of getting this in.
I find myself needing this far less often now, ever since I started using this VS Code extension: "Copy With Imports" https://marketplace.visualstudio.com/items?itemName=stringham.copy-with-imports
Reacted by NoahReacted by Max PatiiukSimple technique to go around this issue:
Let's say you want to move the interface
AppStatefrom the fileapp.tstoapp/types.ts.- Choose
Refactor > Move to a New FileforAppState- that will move the interface toAppState.ts - Rename the newly created file to a unique name, such as
AppStateUNIQUE.ts- this will update all references to this name - Do a global search on
/AppStateUNIQUEand replace it with/app/types - Now simply copy and paste the
AppStateinterface toapp/types.tsand deleteAppStateUNIQUE.ts
This is relatively convoluted to get there but still way faster than manually updating everything because you still take advantage of VSCode automatically updating the imports.
The step to rename to a unique name is only needed if you are moving a symbol with a relatively common name, say just "State". In that case, there will other symbols with this name and it's not safe to do a global search and replace.
Reacted by Josh, jtwigg, Laurent Cozic, Dave Houlbrooke, Matt Trappett, Shoxruxbek, Eldad Bercovici, Jesus Briales, George Andersen, matthewvalentine and 3 moreReacted by Jason Williams, Ben Saufley, Łukasz Orzechowski, Noah, Jeremi Anastaziak, iDschepe, Bruno Capdevila, Adrian, Max Patiiuk and Yuri Teixeira- Choose
I'm not sure of the internals, but this would probably be tricky - to my knowledge there are no refactor/fixits which require users pick a file or provide some sort of input in order for the fixit to run. You might have to make changes to the TS types and have editors accept the new format ahead of getting this in.
This is a valid point, I think it would be worth finding out if fixits can accept user inputs. Or if this could be a command instead?
Laurent Cozic (@laurent22) that solution is very convoluted is probably more easier/faster to use the extension mentioned above to just copy and paste then remap everything that fails.
Reacted by Laurent CozicThe extension just copy the imports to the destination file but it doesn't update references, so not sure how that solves the issue.
This would be a very useful feature! As codebases grow, I find myself wanting to shift and move stuff around quite a bit.
On a related note, I find it a bit annoying that the existing "Move to New File" refactoring guesses the name for you and doesn't pop up a dialog asking you. It uses the name of the class or function as its guess, which I think is reasonable, or as good as it can possibly do, but 9 times out of 10 I'd rather name it something else.
So I think rather than creating a completely new refactoring option, I'd suggest renaming the existing one to "Move to Another File" and have it perform both functions.
The way I envision it, it would pop up a dialog asking what file to move it to which would:
- default to a new file with the name that it guesses,
- allow you to type a new name
- Show auto-completion suggestions of existing filenames
- Create a new file if you press enter with a new filename
- Move to an existing file if you press enter with an existing filename
Reacted by Roman Bekkiev, Light Leung, Ghabriel Nunes, Ha Vu, Milan Korsos, storm1er, Tim Mundt, Artem R, Josh, Andreas Halvorsen Tollånes and 18 more2 remaining items
I, too, would love this feature. I was a bit surprised/perplexed when I went to go do this action, and all that was in the Refactor menu was "Move to new file", and I couldn't find a way to do the more common task of "Move to existing file".
(basically, you have a function, class, interface, etc. in one file, and you want it to be somewhere else :D)
Happy to discuss possible implementation concepts, such as a path box or browse dialog, etc.
I now have to go and move 17 functions by hand, and then somehow fix up their many import usages throughout my project. It could take a while :(
I might have to install abracadabra for this, but it does feel like the kind of useful feature that could be built in.-EDIT-
lol yeah that extension did not work. First time trying to use it, got an error, will have to post an issue on their github :(
Maybe it doesn't work with Typescript properly or something... Anyway, a built-in feature would solve this problem in a much nicer way. Off I go to move them all by hand :'( There goes an hour..."😅 I'm sorry, something went wrong: I can't build the AST from the source code. This may be due to a syntax error that you can fix. Here's what went wrong: Unexpected token (309:39)"
Here's what the VS Code <-> TS Server flow could look like here. I've marked protocol changes/additions with 🚧:
-
VS Code requests refactorings with
getApplicableRefactors -
If the cursors is on a relevant symbol, TS Server returns a
Move to filerefactoring.🚧 Here VS Code needs some way to tell that the returned refactoring is a
move to filerefactoring. This is needed in step 4 so that we know to handle this refactoring differently than other refactorings -
The refactorings are shown to the user and the user selects
move to filefrom the list of refactorings -
Instead of calling
getEditsForRefactor, VS Code now shows the user UI to select which file the symbol should be moved to. We will use a quick pick for this and need to list of files from TS -
🚧 VS Code makes a request to get potential file targets for the move refactor. This should return files in the current project that the selected symbol can be moved to. In addition, TS should return a proposed file name if a new file were to be created.
-
VS Code now will shows this file list in a quick pick. Additionally, we should allow the user to use the file dialog to select another file and an option to create a new file
-
🚧 After the user has selected the target file, VS Code sends a new
getEditsForMoveToFileRefactorrequest to TS Server.This request should include:
- The same fields as
GetEditsForRefactorRequestArgs - The new file name
- The same fields as
-
TS server computes the edit and returns it to VS Code to be applied
During our sync today, we also noted that
move to existing fileis similar to copy/paste the brings along imports (microsoft/vscode#30066). In fact, this refactoring would probably be a good starting point for copy/paste+imports because:- A refactoring has to be explicitly triggered by users. This makes it more obvious what happens if something goes wrong.
- With this refactoring, we're moving entire functions/classes/types instead of selections of code
- However the logic of moving code between files should be very similar between the two
Reacted by sam-s4s, Jilles van Gurp, Ghabriel Nunes, Grant Timmerman, Andrew Brassaw, mariolim96, Henrik Friberg, Nazar Hussain, Roberto Mosca, Jason Williams and 2 moreReacted by Oliver Joseph Ash, Dave Houlbrooke, Michael Engelhard, Daniele Orlando, Henrik Friberg, Jesus Briales, Benjamin Pasero and Jason Williams-
Quick thoughts:
- TSServer should produce the list of files
- TypeScript code should only be able to move to other TypeScript files
- JavaScript code should only be able to move to other JavaScript files
- We probably don't want to offer files in
node_modules - If the UI allows you to go between a file picker and creating a new file, the new file dialog should be pre-populatable with a directory and a file name.
Open Questions:
- How does this work when a file belongs to multiple projects?
- How does this work with an inferred project (which only includes open files)?
Reacted by sam-s4sI agree with all those thoughts. I don’t see a use case for ever moving something into node_modules. 1 step further would be using a .gitignore/ignore list if supplied but I guess there’s been no precedent of TSServer reading from this file up until now. I’d be ok with just node_modules being ignored as a starting point.
Does TSServer already know the list of files or will this impact performance?
For me the most common case has been somewhere else within the same directory, so having the “pre-populated” picker be the directory you’re already in would be a great start. For the sake of performance can it lazily traverse from there if the user requires it?
If the UI allows you to go between a file picker and creating a new file, the new file dialog should be pre-populatable with a directory and a file name.
Do we know if any other language server offers this? I don’t know what the UI would look like for such a feature. A fuzzy search in VSCode would be useful over navigating in and out of directories for example
How does this work when a file belongs to multiple projects?
What’s the definition of a file belonging to multiple projects? Do you have an example?
How does this work with an inferred project (which only includes open files)?
I would expect this to “just work” like before. Is an explicit project required for this feature to work? I assume TS already makes many assumptions on inferred projects.
Reacted by Daniel RosenwasserDo we know if any other language server offers this? I don’t know what the UI would look like for such a feature.
Basically what the Pylance team has prototyped is that "move to..." would be a picker that allows a list of files, along with a top-level option to use the native file picker. The native file picker might allow a pre-populated directory and file name.
What’s the definition of a file belonging to multiple projects? Do you have an example?
A file can be included by another file which belongs to a
tsconfig.jsonproj/ ├── blah/ │ ├── tsconfig.json │ └── a.ts └── blah-tests/ ├── tsconfig.json └── b.tsIf
b.tsimports from../blah/a, thenabelongs to the projects inblah/andblah-tests/.Does TSServer already know the list of files or will this impact performance?
Is an explicit project required for this feature to work?These two are interconnected. Projects have the full file list. Inferred projects only look at open files.
For unconfigured JavaScript projects, you'll often get an inferred project, which means this feature won't really work well if we rely on a fuzzy search of files pre-populated by TypeScript.
Reacted by Mike ClineDaniel Rosenwasser (@DanielRosenwasser) I've updated my proposal for using quick pick to select the target file. Here's some drafts of the relevant protocol additions:
-
Way to identify the
move to file refactoringWe can likely just use
RefactorActionInfo.namewith a name we standardize on -
Way to get a candidate file list shown to user:
interface GetMoveToRefactoringFileSuggestionsRequest extends FileLocationOrRangeRequestArgs { // Pass along the same arguments that we first passed to `GetApplicableRefactorsRequest` } interface GetMoveToRefactoringFileSuggestionsResponse extends Response { body?: { /// Suggested name if a new file is created newFileName: string; /// List of file paths that the selected text can be moved to // TODO: should this be an object instead so that TS could customize how files are shown in the file picker UI? files: string[]; }; }
- Way to apply a 'move to' refactoring after the user has selected a file:
interface GetEditsForMoveToFileRefactorRequest extends FileLocationOrRangeRequestArgs { { // Pass along the same arguments that we first passed to `GetApplicableRefactorsRequest` /// target file path that the user selected (may also be new file) filePath: string; } interface GetEditsForRefactorResponse extends Response { body?: RefactorEditInfo; // TODO: maybe use a new type }
Reacted by kyranjamie, Tim Hutt, mariuszzieja and Noah-
Search Terms
Suggestion
The "move to new file" refactor is really helpful, but what if you want to move the code to an existing module?
This happens very often, when you realise code has been living in the wrong module, or it has a better home in some other module.
I would like to suggest a new refactor: "move to another (existing) file".
My workaround at the moment is to move the code manually, and then manually update all imports, but this is rather tedious!
Checklist
My suggestion meets these guidelines: