jose-elias-alvarez / nvim-lsp-ts-utils

Utilities to improve the TypeScript development experience for Neovim's built-in LSP client.
The Unlicense
438 stars 18 forks source link

Support multiple watch directories #29

Closed jose-elias-alvarez closed 3 years ago

jose-elias-alvarez commented 3 years ago

See #28 for discussion.

The tentative conclusion is to allow watch_dir to be a list of directories, each of which the plugin will check and watch if valid (silently ignoring invalid / nonexistent directories).

The plan is to work on this on the plugin's develop branch, which has recently gotten a bunch of improvements thanks to #27 and will be merged into master as soon as I'm a little more confident about null-ls.

aridgupta commented 3 years ago

See #28 for discussion.

Continuing from the previous discussion. I think the config option would work well for a standalone repo but will it be a robust solution? Suppose I am working on a mono-repo and this repo would have many directories consisting of different features. Then one would need to mention all the directories in the config option and as the number of directories would increase so would the need to change the config. Now in a team, one might work on multiple mono-repos and that would mean cluttering up the config and every time one would need to change the config if a directory is renamed or moved. I think the config option should be set up just once and it should just work irrespective of the project structure.

jose-elias-alvarez commented 3 years ago

I feel like the cleanest solution for complex situations involving multiple mono-repos would be to have users set watch_dir to "", then have the watcher respect .gitignore files, since they're as close to universal as we can get. (This would be in addition to the option to set watch_dir as a list, which I think is a good solution for small-to-medium projects.)

Plenary's scandir module has a respect_gitignore flag, so we could use that to get all non-ignored directories at the top level, then run a separate watcher for each. We'd still have to test the performance impact of running multiple watchers (and I'd appreciate help with this, since I don't personally work on any large projects), but I think this would be the most lightweight option.

I'm also thinking of eventually spinning off the file watching functionality into a separate library-style plugin, so I think that would be the most flexible solution for other use cases, too. What do you think?

aridgupta commented 3 years ago

Yes, you're right. Using .gitignore would be as close to a universal solution as possible.

I think the watch_dir option should always be set to "" and if a user chooses to watch specific directories then he/she could just provide the directories in a list. Since almost all projects would use .gitignore, I don't think the option to provide a list of directories is needed but it should be there as an option.

I can help with user testing but I don't work on very large repos but I do work on a mono repo which I could use to test the plugin.

jose-elias-alvarez commented 3 years ago

Hmm, you're right that it could actually work as sensible default. Maybe watch_dir could then be a fallback option that accepts a single directory or list of directories to watch if the root directory doesn't have a .gitignore file, which would work for small projects that aren't set up as Git repositories.

I'm prototyping this locally and will update the issue when the feature is available to test, but it's working pretty seamlessly so far (I'm using my own projects and this repo to test it). I also want to put in a PR to fix a couple of Plenary bugs and make sure .gitignore works properly with directories.

jose-elias-alvarez commented 3 years ago

The Plenary maintainers moved fast on the PR, so I was able to get this up and running for now, and it's available on the develop branch.

It's working well on the sample repo and on my work projects, but the question is how it will perform when watching 2+ directories simultaneously. Could you give it a try when you can? If you set debug = true in your plugin setup function, you'll be able to see in :messages if the watcher is active on the expected directories, too.

aridgupta commented 3 years ago

it is currently watching all the directories but it's including node_modules as well. However, formatting isn't working for me.

jose-elias-alvarez commented 3 years ago

Hmm, that's strange. What is the format of your .gitignore like? I think think the Plenary module we're using doesn't support complex patterns, but node_modules, /node_modules, and node_modules/ should all work.

For formatting: the null-ls integration hooks into the Neovim LSP client, so if you've set enable_formatting = true, it should format the current file when you run vim.lsp.buf.formatting(). If that doesn't work, could you open up another issue so we can troubleshoot?

aridgupta commented 3 years ago

My .gitignore has /node_modules.

Do I need to install the null-ls plugin? If so, what do I put in the sources option while setting it up?

jose-elias-alvarez commented 3 years ago

Does the root dir resolve correctly in your project? You should see a message that says attempt to watch root dir ... in :messages. If the root dir looks okay, could you tell me where node_modules is located relative to the root? (I don't know anything about monorepo structure, but I'm working under the assumption that node_modules is in the root dir.)

Also, are you running the latest version of Plenary? (If you're unsure, I would comment it out, run your plugin manager's clean command, then reinstall.)

If those don't work, could you try cloning the sample-monorepo and verifying that it correctly ignores node_modules? If so, I think there's something about your project setup that we have to account for.

File watching doesn't depend on null-ls, but formatting does. You'll need to install the plugin, then add require("null-ls").setup {} somewhere in your config. This plugin should take care of the rest.

aridgupta commented 3 years ago

Also, are you running the latest version of Plenary? (If you're unsure, I would comment it out, run your plugin manager's clean command, then reinstall.)

After updating Plenary, it successfully ignores node_modules.

If the root dir looks okay, could you tell me where node_modules is located relative to the root? (I don't know anything about monorepo structure, but I'm working under the assumption that node_modules is in the root dir.)

Not all monorepo setup will hoist the node_modules folder to the root. Lerna uses a root node_modules and also every user defined package has a node_modules folder.

File watching doesn't depend on null-ls, but formatting does. You'll need to install the plugin, then add require("null-ls").setup {} somewhere in your config. This plugin should take care of the rest.

Formatting still doesn't work. Using the default option prettier.

jose-elias-alvarez commented 3 years ago

Thanks for confirming! The monorepo issue is concerning (we'll probably have to recursively check for sub-root directories within the root directory and scan each), but I'm going to wait until it comes up to deal with it and consider this closed for now.

For formatting, could you open up another issue and post your full config for this plugin and for null-ls? Thank you for your help and patience!