Skip to content
This repository has been archived by the owner on Nov 12, 2022. It is now read-only.

Add some metadata to the servers #90

Closed
kylo252 opened this issue Sep 13, 2021 · 24 comments
Closed

Add some metadata to the servers #90

kylo252 opened this issue Sep 13, 2021 · 24 comments

Comments

@kylo252
Copy link
Contributor

kylo252 commented Sep 13, 2021

It's pretty useful to be able to query the package_name when it's applicable. This will allow for some flexibility and easy to implement features, here are some of them:

  • very useful for debugging, since some servers may have multiple distributions, see: https://github.com/hrsh7th/vscode-langservers-extracted
  • ability to reuse a server that already exists on path
  • allow installing/updating globally, but still without requiring sudo, i,e: pip install --user and so on
  • you can uninstall a package by name instead of nuking the folder
  • use the package full name in the info panel instead of the abbreviated name used by lspconfig. (since you actually need npm install bash-language-server for bashls)
  • allow an easier way to fetch metadata for some fancy LspInstallInfo per server

Let me know what you think :)

@williamboman
Copy link
Owner

williamboman commented Sep 15, 2021

Hey! This is definitely on my mind as well. I do want to be careful adding certain features, especially those that incur a lot of added complexity. It'd take a lot of convincing and then some more for me to consider allowing global/system-wide or even user-local installations (really, anything outside of nvim-lsp-installer's dedicated file system boundaries). But maybe I misunderstand your 2nd and 3rd points?

  • you can uninstall a package by name instead of nuking the folder

I'm not following this one, could you perhaps give an example?

  • use the package full name in the info panel instead of the abbreviated name used by lspconfig. (since you actually need npm install bash-language-server for bashls)

Yeah some of the lspconfig names are pretty opaque in terms of what's actually installed. I definitely think it makes sense to be more transparent with what'll actually end up being installed. I have an idea where this kind of metadata would quite easily be able to be inferred from each server's installation steps. For example, a server that has a npm.packages { "typescript-language-server", "typescript" } installer configured, one could easily infer that:

  1. a Node.js runtime with npm installed is required to install this server
  2. npm package typescript-language-server will be installed
  3. npm package typescript will be installed

allow an easier way to fetch metadata for some fancy LspInstallInfo per server

Yeah, I'm wondering what would be interesting to display here. I recently added a relative timestamp showing when each server was installed. I think this is the cheapest outlet to experiment with :)

All in all, there's only really two features remaining before I feel like this plugin is more or less feature complete, and that is 1) a nicer API to ensure that a set of provided servers are installed (easy to impl), and 2) functionality that allow you to upgrade servers when upgrades are available (either through some UI notification, or automatically behind the scenes). Anything else feels somewhat redundant

@kylo252
Copy link
Contributor Author

kylo252 commented Sep 16, 2021

I actually retract the idea of user-level install because you don't want to be dealing with people who didn't setup their package managers correctly. In a perfect world, the user would have configured all the correct paths for their tools, but that's sadly not the case: LunarVim/LunarVim#1468, LunarVim/LunarVim#1463 😢


Regarding the other points, let's take this example.

:LspInstall sumneko_lua pyright yamlls jedi_language_server tsserver

Here's what I got:

lsp_servers
├── jedi_language_server
│   └── venv
├── python
│   ├── node_modules
│   ├── package.json
│   └── package-lock.json
├── sumneko_lua
│   ├── [Content_Types].xml
│   ├── extension
│   └── extension.vsixmanifest
├── tsserver
│   ├── node_modules
│   ├── package.json
│   └── package-lock.json
└── yaml
    ├── node_modules
    ├── package.json
    └── package-lock.json

It might be good if jedi were to use the python folder as well. But the is currently not attainable, because the pyright ninstaller doesn't know how search for pyright anymore. It will resort to a more aggressive approach by removing the root folder, in this case python, completely instead of relying on the package manager to clean it up.

All in all, there's only really two features remaining before I feel like this plugin is more or less feature complete, and that is 1) a nicer API to ensure that a set of provided servers are installed (easy to impl)

This is why I feel this such an easy fix/feature. Consider this, which will be even easier internally:

fd 'package.json' --max-depth=2 | xargs dirname | xargs -I {} npm ls -C {}

# 	  results

#	  python@ /home/hatsu/.local/share/nvim/lsp_servers/python
#	  └── pyright@1.1.169
	  
#	  tsserver@ /home/hatsu/.local/share/nvim/lsp_servers/tsserver
#	  ├── typescript-language-server@0.6.2
#	  └── typescript@4.4.3
	  
#	  yaml@ /home/hatsu/.local/share/nvim/lsp_servers/yaml
#	  └── yaml-language-server@0.22.0

Do you see how it would make things just a slightly bit easier to display and validate that we're using v0.22.0 of yaml-language-server. You can now do npm rm yaml-language-server and then be free to remove the folder afterwards.

Other observations:

  • Again, this is not really specific to node modules, it just so happens that most servers are written in node, for better or for worse. And even though npm is slow, it can still give you some useful info npm view yaml-language-server. Hopefully there's some fast Rust tool that's good at dealing with nodejs mess :)
  • This will allow for less hacky ways to pin a version of language-server, use a different source/fork or any other useful options that can be passed to the package manager.

@williamboman
Copy link
Owner

It might be good if jedi were to use the python folder as well. But the is currently not attainable, because the pyright ninstaller doesn't know how search for pyright anymore. It will resort to a more aggressive approach by removing the root folder, in this case python, completely instead of relying on the package manager to clean it up.
[...]
Do you see how it would make things just a slightly bit easier to display and validate that we're using v0.22.0 of yaml-language-server. You can now do npm rm yaml-language-server and then be free to remove the folder afterwards.

I'm not following why you'd either a) want to put this kind of "semantic" state in the file system hierarchy in the first place, and b) why exactly where servers end up being installed is a concern for the (average) end user (other than the guarantee that it's inside the data stdpath [i.e., not system-wide, globally, or user-local]).

The fact that there exist a python directory name (which is allocated to pylsp iirc) is just a legacy thing. In fact soon the default behavior will be that these directories will take the same name as their corresponding lspconfig name (apart from the existing servers with non-matching names, to avoid breaking changes).

Don't you think it'd be much easier to contain this kind of state/logic in the Lua layer? Or I might still be misunderstanding.

Do you see how it would make things just a slightly bit easier to display and validate that we're using v0.22.0 of yaml-language-server. You can now do npm rm yaml-language-server and then be free to remove the folder afterwards.

Perhaps from a shell scripting perspective. I think the same can easily be implemented in a platform-agnostic manner if each server was responsible for reporting such metadata (so for npm-based installations we'd spawn subprocesses that report back a single package version). This could easily be parallelized with libuv.

Again, this is not really specific to node modules, it just so happens that most servers are written in node, for better or for worse. And even though npm is slow, it can still give you some useful info npm view yaml-language-server. Hopefully there's some fast Rust tool that's good at dealing with nodejs mess :)
[...]
This will allow for less hacky ways to pin a version of language-server, use a different source/fork or any other useful options that can be passed to the package manager.

If my quick check was correct, 23/50 servers are currently installed via npm, so it's actually not even a majority. But yeah I think the extra capabilities each server would offer (versioning, pinning, whatnot) will be entirely constrained by how it's installed in the first place. npm-based distributions can be heavily standardized whereas servers that require custom build scripts will not.

@williamboman
Copy link
Owner

williamboman commented Sep 22, 2021

I've started experimenting with this now, preview: https://asciinema.org/a/Bj6Q03SweLj6gJ7EGNqIr4beU. This is actually pretty easy to implement thanks to the state-driven UI framework that is in place. The question is what's interesting to show.. I'm thinking the following, for starterss:

For installed servers:

  • Installation date
  • Installed version
  • Latest available version (perhaps also do some diffing and highlight this if it differs from the currently installed version)
  • LSP server homepage (e.g. its GitHub repo or public website if more suitable)

For uninstalled servers:

  • Version that would be installed
  • What exactly will be installed (this one I might postpone til a later iteration)
  • LSP server homepage (e.g. its GitHub repo or public website if more suitable)

Anything else that might be interesting to display?

@wookayin
Copy link

This looks like a great feature. Although obvious, how about displaying the full installation path (the LSP server directory or the cmd executable) so that one can navigate if they'd want some troubleshooting?

@williamboman
Copy link
Owner

That makes sense too I think! I'd probably opt for the server directory itself rather than the full command. The full command should be available via :LspInfo already, I also hope to be able to remove these cmd overrides from this plugin altogether in the future:tm:.

@kylo252
Copy link
Contributor Author

kylo252 commented Sep 22, 2021

I've started experimenting with this now, preview: asciinema.org/a/Bj6Q03SweLj6gJ7EGNqIr4beU.

I'm actually working on implementing something similar just now

Demo

j0ZtZSq0UK

There was a small problem where it would try to attach the server immediately if I had the a file with the associated filetype opened.

Race condition(?)

P3enb02pVa

The solution was disabling vim.cmd [[ do User LspAttachBuffers ]].

@williamboman
Copy link
Owner

williamboman commented Sep 22, 2021

That's so cool 😎 ! However this seems to be a tighter integration between LunarVim and nvim-lsp-installer, and not so much about extending the feature set of nvim-lsp-installer itself - or am I missing something 🤔 ?

Regarding the race condition - this might be because the .setup() function of the server is called before the server is successfully installed? vim.cmd [[ do User LspAttachBuffers ]] will essentially only trigger lspconfig to attach all existing buffers to potential server candidates that have been registered with lspconfig (i.e. through .setup())

@williamboman
Copy link
Owner

williamboman commented Sep 22, 2021

I also have plans on adding something similar to what you're doing actually! I just pushed a somewhat finished version of it to ensure-installed-servers

@kylo252
Copy link
Contributor Author

kylo252 commented Sep 22, 2021

Oh my bad, I misread the command name and thought it was an auto-installer!

The two features that I would like to see, and can even help with, are:

  1. allow dismissing the split quickly and also allow the use of an actual popup
  2. it has been mentioned before, but I'd like to see some kind of feedback from the installer that is not coupled to the UI state.

I feel like the last point might be tricky, but I hope not.

@williamboman
Copy link
Owner

williamboman commented Sep 22, 2021

  • allow dismissing the split quickly and also allow the use of an actual popup

Yeah now most interactions with the plugin will be centered around the UI window. Implementing a floating window instead of a regular vertical split should actually be pretty straight forward. The code is a bit so-so and not really considered stable yet, but this is the part responsible for opening the window. I'd like to avoid having options for everything so I think either a floating window or vertical split window should be supported, not both. I don't have any strong opinions so happy to receive PR changing to floating window!

2. it has been mentioned before, but I'd like to see some kind of feedback from the installer that is not coupled to the UI state.

This should be possible today. The drawback is that if you bypass the APIs that are attached to the UI window, the UI window will become out of sync with any operations done outside of it. What you're interested in would be Server:install_attached(). Here's how the UI calls that function, for reference.

edit: Everything's technically possible:tm:, so we could make sure to funnel everything through some centralized state management while allowing for 3rd party use cases (basically lift the state management from the status-win/init.lua module and spread it globally across the plugin). I'm not too compelled to make such a change at this point in time though, I'd like things to stabilize a bit more.

@kylo252
Copy link
Contributor Author

kylo252 commented Sep 22, 2021

edit: Everything's technically possible™️, so we could make sure to funnel everything through some centralized state management while allowing for 3rd party use cases (basically lift the state management from the status-win/init.lua module and spread it globally across the plugin). I'm not too compelled to make such a change at this point in time though, I'd like things to stabilize a bit more.

Yeah that's understandable, take a look at #98 and see if it's good enough.

@williamboman
Copy link
Owner

Some more progress on this: https://asciinema.org/a/fmqCxtLUR4HrqLcbteNaMGQqR

@kylo252
Copy link
Contributor Author

kylo252 commented Sep 24, 2021

Some more progress on this: asciinema.org/a/fmqCxtLUR4HrqLcbteNaMGQqR

How's the performance? I messed around a bit with yarn which seems slightly faster in fetching data, but the initial setup is way too annoying compared to npm. I can make a PR for it if you'd like to see/support it.

@williamboman
Copy link
Owner

Do you mean in terms of latency? It's perfectly fine for me, although the "latest version" is network-bound so ymmv. What happens is that when you expand a server it'll gather each individual data point separately in a subprocess via libuv, so in terms of perceived performance I don't think yarn vs npm will matter

@kylo252
Copy link
Contributor Author

kylo252 commented Sep 24, 2021

Do you mean in terms of latency? It's perfectly fine for me, although the "latest version" is network-bound so ymmv. What happens is that when you expand a server it'll gather each individual data point separately in a subprocess via libuv, so in terms of perceived performance I don't think yarn vs npm will matter

Good to know! I've heard conflicting tales about the superiority of yarn. However, I could not get yarn init to shut up about either the license file or the default settings. It's made to be used interactively in the first place, smh..

@williamboman
Copy link
Owner

Hehe maybe you need some NON_INTERACTIVE env set 🤷‍♂️. Another reason to stick with npm is that it's generally shipped together with Node, whereas Yarn is less so (maybe this has changed). I do want to enable the possibility to run server installations via Yarn instead of npm, as npm has a tendency to cause issues when on corp VPNs/proxies. But that's a separate topic

@williamboman
Copy link
Owner

Note to self: I realized for node packages we could probably bypass npm entirely through something like:

local package_json = vim.json.decode(fs.read_file(path.concat { server.root_dir, "node_modules", package, "package.json" })
package_json.version

@williamboman
Copy link
Owner

And for fetching latest versions https://api.npms.io might be more interesting than going through npm cli, like:

local package = vim.json.decode(fetch(("https://api.npms.io/v2/package/%s"):format(package_name)))
local latest_version = package.collected.metadata.version

@kylo252
Copy link
Contributor Author

kylo252 commented Oct 1, 2021

And for fetching latest versions https://api.npms.io might be more interesting than going through npm cli, like:

local package = vim.json.decode(fetch(("https://api.npms.io/v2/package/%s"):format(package_name)))
local latest_version = package.collected.metadata.version

I recently learned that npm view fetches data from the online registry, which makes it slow. Is that the reason why you don't want to go through local npm calls?

@TeoDev1611
Copy link
Contributor

Note to self: I realized for node packages we could probably bypass npm entirely through something like:

local package_json = vim.json.decode(fs.read_file(path.concat { server.root_dir, "node_modules", package, "package.json" })
package_json.version

Can you check this can be help for dont use the api :)

https://stackoverflow.com/questions/10972176/find-the-version-of-an-installed-npm-package

@williamboman
Copy link
Owner

Closing this thread as we've come pretty far in terms of displaying metadata, which was the topic of this issue.

As for what was discussed for version checking of servers to allow updating outdated ones - this will be continued in #194.

@wookayin
Copy link

wookayin commented Oct 24, 2021

What about versions? (currently installed version as well as latest, #194) This one would be another important metadata to display.

@williamboman
Copy link
Owner

Yeah that won't be forgotten in #194! As long as the data is available, it's really easy to add to the UI

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

No branches or pull requests

4 participants