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

"Move to file" code action #221

Closed
unphased opened this issue Jan 13, 2024 · 14 comments · Fixed by #224
Closed

"Move to file" code action #221

unphased opened this issue Jan 13, 2024 · 14 comments · Fixed by #224

Comments

@unphased
Copy link

unphased commented Jan 13, 2024

I have been on a quest to bring the most useful vscode functionality into neovim, that being the recent "move to file" code action.

This is a similar code action to the "Move to a new file" which I can confirm work great under both nvim-lspconfig and typescript-tools.nvim (this plugin) as providers for tsserver (typescript-language-server).

As you might imagine, moving some code into a target existing code file is a more common use case than extracting it out into a new file. Although I'm also happy to report that moving the newly created file using nvimtree with the https://github.com/antosha417/nvim-lsp-file-operations plugin allows for very streamlined workflow. it's just that moving code to a new file isn't possible yet.

I'd like to understand what we have to implement in order for this to become a reality. I refuse to open vscode just to do refactors but that is what i have to resort to right now.

I did some poking around and I found that this capability is a VSCode specific functionality and isn't provided by tsserver: https://github.com/microsoft/vscode/blob/1c0c4726d30a0574739b35cc6d7a7dea9b3decb2/extensions/typescript-language-features/src/languageFeatures/refactor.ts#L146

@pmizio
Copy link
Owner

pmizio commented Jan 14, 2024

Hi, thanks for investigation, it should be relatively easy to do looking at code present in vscode extension.
In this extension I see it as custom lsp request + command(eg. :TSToolsMoveToFile). One of challenges I see is providing target file, maybe some telescope.nvim integration will be convenient.
I'll try to look deeper into it next week.

@unphased
Copy link
Author

unphased commented Jan 14, 2024

I agree that is a very reasonable way to implement it. I did find that your :TS based commands are one area that exceeds native nvim-lspconfig tsserver support!

this code action ideally will be used mostly with ranges, I am pretty sure that commands can be called on ranges though. Then again if we can target a file quickly (ideally we can have it default select the previously selected target file!) then moving stuff one at a time rapid fire without using ranges would also be elegant.

Yes telescope file picker is an easy default to start with.

BTW I will cross link to the discussion I have in another similar project. I really want to enable this code action!!! yioneko/vtsls#26

I'm a bit curious what is the difference in approach between typescript-tools.nvim and vtsls, in the latter they maintain patches against vscode's typescript extension code, it's not clear to me yet how typescript-tools.nvim works, but from your readme it certainly sounds similar. If you are porting vscode extension functionality from ts to lua I worry somewhat about the maintenance burden. But ... maybe it isn't a big deal?

@pmizio
Copy link
Owner

pmizio commented Jan 15, 2024

@unphased I'm happy to tell you I successfully created POC(range is hardcoded, there is no error handling) of this feature with telescope integration. Short video, I'll try to finish this this week:

feature.mp4

@pmizio
Copy link
Owner

pmizio commented Jan 15, 2024

About differences between this plugin and vtsls:
My plugin start up raw tsserver binary and teaches nvim how to speak over its protocol which is n't compatible with LSP one (nvim runs tsserver).
As far as I know vtsls is minimal wrapper over vscode or it's extension host which fill up vscode apis to make them talking over LSP instead of calling vscode gui things(nvim runs vtsls runs tsserver).
So in the end differences are significant but they are just different approaches to solve same problem. In favor of mine plugin is fully lua running inside nvim so integration is closer and allows me for more hacks, but vtsls is more generic and it can be used in any editor with LSP support.
About maintenance burden, honestly besides reverse engineering things and scattering vscode code to understand it initially it isn't any, from time I published plugin I barely touched core/engine of plugin.
And after all writing this was a lot of fun and I learned a lot of new things.

@unphased
Copy link
Author

unphased commented Jan 17, 2024

@pmizio thank you for taking a look at this! I am very excited to share that (apparently on my goading) we will soon have move to file (the most powerful code action by far IMO) implemented in two different approaches from both vtsls (via nvim-vtsls) and typescript-tools.nvim. I am confident this is a feature that will help motivate more folks to join the neovim ecosystem, and also paving the path toward bringing these powerful typescript capabilities to even more code editors. Like the development environment that I'm building.

I can also say from my (limited) testing so far both this plugin and vtsls provide comprehensive typescript refactor capabilites beyond that provided by the basic LSP conformant implementation. Just a week ago I was still marveling at how amazing the "move to a new file" code action is. And now that both will support move to file, I'm spoiled for choices!

I guess what you're saying is you basically review how vscode extension code does the job, and you literally reimplement it in lua. Which is pretty awesome. I haven't had the time to dig into it but it does strike me as pretty heavily abstracted, so maybe this is more plausible than i thought at first.

@pmizio pmizio linked a pull request Jan 17, 2024 that will close this issue
2 tasks
@pmizio
Copy link
Owner

pmizio commented Jan 17, 2024

@unphased so I created draft pr with functionality, I was able to preserve this code action as code action itself without additional TS command. I have to add two additional config things but feature itself is usable now and available for tsserver 5.2 and beyond.
So if you want to try it be my guest.

@unphased
Copy link
Author

I gave it a spin, pointed Lazy at your new branch and it works like a charm!

@unphased
Copy link
Author

unphased commented Jan 19, 2024

questions

  • do you think it makes sense to pipe the refactor insertions through = to format it? I get something silly like 8 space tabs. not a big deal tbh
  • would you happen to know if it is normal (e.g. if vscode also does this) when you have a function defined in B.ts that is being imported and called from A.ts, and you go move that function with this code action from B.ts into A.ts, it leaves the import for it to itself in A.ts. It actually goes to edit the import { func } from './B.ts' into import { func } from './A.ts' inside of A.ts, which obviously will lead to an error trying to simultaneously import and export one thing from a module. All this needs to do instead is realize A.ts is self and eliminate these self import statements. It ought also to change it from exported to non exported if not referenced from elsewhere...

@pmizio
Copy link
Owner

pmizio commented Jan 22, 2024

@unphased

  1. You probably have someting wrong in your configuration of tabs vs spaces and so on we in this plugin get data about indenting from nvim and set same ones for opened file in tsserver here. In my config I get everything correctly formatted after move - my indentation settings:
local set = vim.opt

set.tabstop = 2
set.shiftwidth = 2
set.expandtab = true
  1. Probably same error exists on vscode side, in here I apply edits provided by tsserver nothing more as far as I looked into code from vscode you posted vscode do the same thing. Checked and vscode also do it.

@unphased
Copy link
Author

unphased commented Feb 8, 2024

@pmizio I made progress on that issue. I saw that somehow about 80% of the time the indentation becomes 8 space expanded tabs, while my configuration is always with 2.

Today I realized on a whim that gq is also supposed to format, and when i try it I get 8 space tabs!!! So, somehow, (this makes no sense) my visual mode = and gq<motion> apply different operations. my ts, sw, and et vim settings are all set properly to 2 and expandtab. I will continue to investigate as this is really silly and i do need to restore the functionality of my normal mode format (gq) behavior. I am confident if I do that this problem will disappear.

BTW i've been daily driving this plugin and using this code action a lot. It is really helpful and I already got through a massive refactor that I wouldn't've had the patience for without it.

I did notice at some times there is a lua error that happens preventing the code action from executing properly. it most often happens when i create a new empty file and target it with this thing. I will provide a stack trace the next time I encounter it.

@unphased unphased closed this as completed Feb 8, 2024
@PeterCardenas
Copy link

can we re-open this issue since the PR has not been merged? @pmizio

@unphased
Copy link
Author

unphased commented Jul 1, 2024

I have been using this thing for a while. I think it got put into main/master at some point. My mistake. I was using this branch the whole time. Let's merge #224 😃

@unphased unphased reopened this Jul 2, 2024
@yuriteixeira
Copy link

Any updates here? This is a really useful feature, can't wait to have it.

@unphased
Copy link
Author

unphased commented Dec 1, 2024

I am sad to report something about this is not working and it feels like it coincided with this merge. But I am not sure because I am messing with typescript due to needing to use vue/volar in my work project. However, i can confirm also that clearing out all my mason-lspconfig setup does not make it work again either. Anyone else?

Update: Looks like the code move I was attempting to make also fails under VS Code so i will keep playing with it to see what's going on. I guess I thought this (upstream move-to-file) feature was more fully baked than it is or it is suffering from regressions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants