Add an LSP to be able communicate with editors #94
Conversation
|
There already exists a package that aims to do the same and has more complete support for LSP: #21 (comment) |
|
@rchl From what I could see, the ideas in both are pretty different, on my PR the idea is to integrate an LSP into this package, so that it can be used not only by the vscode extension itself, but also any other editor extension, and have an output option, so we can support whatever the editor needs. Having an LSP is a bit nicer in my opinion, as the editor can spawn the process in the background, keeping it running and then communicate with it via RPC when needed, without needing to spawn new instances on each request (which probably makes it a bit faster too), and I believe that using the standard would make it easier for editors to adopt. |
|
Personally I think this code should be integrated with typescript language servers (like I don't even see how your approach would even be possible in an editor like SublimeText (which I'm using) because that would means that the diagnostics from one server would have to be passed to another server and then shown on hover (and/or diagnostics panel). This doesn't really seem feasible to me.
LSP spec supports markdown and plaintext only so it makes sense to output in markdown as a main format. Of course it would also makes sense to support plaintext output if neovim doesn't support markdown but is that really the case? The author of that version has made it for neovim in mind, as far as I can tell - hexh250786313#2 (comment) (there is mention of it not supporting inline links but code block seem to be supported) |
|
I agree having this integrated directly on
This seems to be what the VSCode extension does though, from this code, the extension checks the diagnostics when they change, and then formats it, returning new diagnostics (which is why we see two diagnostic errors when we install this extension, as seen on this video). On my neovim plugin, because neovim exposes apis to control the visibility of all diagnostics, I did a workaround to filter the diagnostics from tsserver that we have already processed, so that we don't show them twice (once from typescript, and the other from pretty-ts-errors). I'm not sure if theres a better way, other than integrating this directly onto
I also assumed it did support markdown as well at first, but when I passed markdown to it, neovim rendered it as plain text, and when searching their docs they don't mention anything, so without looking at the coc plugin (and also by the screenshots he provided), I assume he's not using the standard diagnostics from neovim (probably he's opening a new floating window instead and manually rendering the output there). |
I'm not sure if we are on the same page. The
VSCode is not a great example because it doesn't run tsserver through LSP while other editors do (well, coc-tsserver maybe not). All we really need here is a package like the one I've linked that provides a nodejs-level API that returns LSP-compatible output. Ideally this package would include such code and expose an API to format the diagnostic. But not sure if the author of this one wants to maintain a markdown variant in addition to the original one. |
In fact, my
As for
|
I will continue to port new changes from the original until it has its own markdown version. |
|
@hexh250786313 hey, thank you for your input as well! It's nice to see some traction on this issue 馃槂
The main reason why I decided to filter the original messages on the diagnostics panel is that otherwise we would have duplicated messages signaling the same thing (and I've seen multiple people commenting negatively about this behavior), and while I agree that being able to control that on the original LSP is the best solution, I don't think we can do that currently, hence the current filter on the plugin. Of course, if needed we can toggle that on/off, I just personally feel like displaying the same error twice is a bit weird
Because I didn't see a PR about porting your changes to this repo, or discussing the idea of doing that, I imagined you wanted to keep your fork as its own separate thing, which is why I created this one trying to push forward the LSP integration, so every editor can benefit from those features (be that vscode, neovim with the native LSP or with the coc's LSP, sublime, etc). I believe that having a strong foundation holding the core functionalities and allowing editors to leverage those while implementing editor-specific plugins separately is probably the best bet, and it's actually why LSPs were invented in the first place. By having separate solutions, we will need to worry about keeping both versions up to date with each other, and sometimes that can be a nightmare, especially on big refactors...
Thank you for the tip, depending on how this PR gets handled I can also explore the In any case, I guess we are now mostly discussing plugin-specific topics, but the main idea behind this PR was to try to push forward the idea of having a solution that could fit all editors and discuss how we can implement those flexibly, since currently, we have a solution which is very tailored to vscode, and since this was a highly anticipated feature but with little movement, I decided to give it a shot here |
Hey 馃憢
This is an attempt to make the extension available to all editors that support LSP (In my specific case, I'm testing this with Neovim), so that we can close #21 and #26. For now, this is mostly a draft, just to discuss some ideas on how we could make this possible, but hopefully, we can push this forward 馃槃 .
The initial idea I had was to for now just add an LSP to this repo and call the same methods that the vscode extension calls, but ideally, it would be good to separate the LSP module from the extension module, so that other editors can import only the lsp, but not the vscode extension (and on vscode we can probably just import the methods directly from the lsp package and use them as we are currently using). I saw that there's a PR to transform this repo into a monorepo, so it is also something we could leverage later to separate both modules
I've also created a draft plugin for neovim which uses this LSP (available here), and while it's still far from being usable, we can already get the results from the LSP:
As you can see, there is still one problem which is that Neovim doesn't render HTML nor Markdown on its diagnostics, just plain text, so we would need to also have the option to choose which output type we should return the diagnostics in, when making the LSP call.
My initial idea for that was when calling the LSP from the plugin's editor, we also pass the expected output on the request, then on the LSP we use that and format things accordingly. The downside is that we would need to pass around the output to almost every function, which is a bit annoying.
Another option would be to create providers that exports all the format functions for the particular output type, but that I guess would require a bigger refactor of the codebase.
What do you think?