-
Notifications
You must be signed in to change notification settings - Fork 95
feat - improve support for monorepos and fvm in monorepos #476
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
Conversation
…ction is no longer used
I tried to test but I get an error on opening: Error in .nvim.lua:
Lua: ...lazy/flutter-tools.nvim/lua/flutter-tools/utils/path.lua:57: invalid value (boolean) at index 1 in table for 'concat'
stack traceback:
[C]: in function 'concat'
...lazy/flutter-tools.nvim/lua/flutter-tools/utils/path.lua:57: in function 'join'
...y/flutter-tools.nvim/lua/flutter-tools/lsp/fvm_utils.lua:26: in function 'flutter_bin_from_fvm'
...lazy/flutter-tools.nvim/lua/flutter-tools/executable.lua:106: in function 'get'
...e/nvim/lazy/flutter-tools.nvim/lua/flutter-tools/dap.lua:7: in function 'setup'
...share/nvim/lazy/flutter-tools.nvim/lua/flutter-tools.lua:65: in function 'start'
...share/nvim/lazy/flutter-tools.nvim/lua/flutter-tools.lua:129: in function 'setup_project'
[string "<nvim>"]:1: in main chunk |
I’ve been using this branch for some time and it seems to work well. However, I’m not comfortable merging so many changes in a single PR, especially since it’s fairly large and touches many core systems. Would it be possible to split this into smaller, more focused PRs? That would make the review and integration process smoother and safer. |
Ok. I will try to do so within the week. I am thinking of splitting bug fixes unrelated to new code into their own PR, the changes related to not using LSP's root dir as cwd into its own PR, and also give the FVM changes its own PR. Does that sound reasonable or should I also split the FVM changes? |
Yes, i.e. refactorings that just move code around could have a separate PRs, these should be safe to merge. Also, I see you added require("flutter-tools").setup_project({
{
name = "Mobile",
},
{
name = "Web",
device = "chrome",
web_port = "4200",
},
})
|
web_port relates to the lsp diagnostic server's web port: Diagnostic Server, which as far as I am aware is different from the existing web_port project config which I believe relates to the flutter dev tools port? Regardless, I should probably change its name from web_port to lsp_diagnostic_port or something along those lines. The webport commit was not supposed to be in this PR. |
flutter-tools uses the active buffer's LSP client's root_dir as the cwd for executing commands. This is fine for a single project repository as the root_dir is set to be that project's root directory. But when working with monorepos this approach does not work, as the LSP's root_dir is whatever project was first opened when attaching the LSP.
Also when using FVM, flutter-tools does not consider the possibility of having multiple flutter versions, and instead keeps attached
the first LSP that it found.
To properly support monorepos we then have to do two things:
for a .fvm folder, and then using that folder's flutter sdk.
This PR does precisely that, while also moving things around to keep code clean, here is a list of the changes done by scrolling-down-to-the-end-of-the-pr order:
root directory for the LSP
project root patterns until it finds the first one, which will then be used as the root directory for commands.
If it is unable to find a root directory by that method, we fallback to the old behaviour of returning the active lsp's root_dir
When using FVM this PR changes flutter-tools so that it might run multiple Dart LSP servers, I thought about giving an option of keeping only the most recently launched LSP and closing the rest since the Dart LSP can be quite resource intensive, but didn't go through implementing that since I don't really have the time right now.