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

Add an LSP to be able communicate with editors #94

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

GabeDuarteM
Copy link

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:

image

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?

@rchl
Copy link

rchl commented Feb 25, 2024

There already exists a package that aims to do the same and has more complete support for LSP: #21 (comment)

@GabeDuarteM
Copy link
Author

@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.
On the PR you mentioned, the package is supposed to be a completely separate thing that replaces the HTML rendering that the original package provides with Markdown ones (which I believe is not completely supported by neovim as well, as shown on the screenshot above). It also exposes a CLI that can be interacted with, which could be used by extensions to communicate, but as far as I know, that doesn't follow the LSP convention, so we need to do additional process spawns by ourselves on the editor extension when we need to call the format functions.

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.

@rchl
Copy link

rchl commented Feb 25, 2024

Personally I think this code should be integrated with typescript language servers (like typescript-language-server or coc-server) and not be a separate server. For that, an approach where there exists a package that outputs in LSP-format (like the one I've linked to) is what is really needed. Then it's up to maintainers of those servers to integrate with it as an optional feature.

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.

On the PR you mentioned, the package is supposed to be a completely separate thing that replaces the HTML rendering that the original package provides with Markdown ones (which I believe is not completely supported by neovim as well, as shown on the screenshot above).

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)

@GabeDuarteM
Copy link
Author

I agree having this integrated directly on typescript-language-server would be the best thing, but I guess this is exactly the reason why pretty-ts-errors exists, to bridge this gap and output errors that are easier to understand. If the errors on tsserver were better formatted, then we wouldn't need any of those packages 😄

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.

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 typescript-language-server, as you mentioned before 😕

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 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 can have a better look into his plugin later though, to confirm this is really the case, thank you for bringing this up 😄

@rchl
Copy link

rchl commented Feb 26, 2024

I agree having this integrated directly on typescript-language-server would be the best thing, but I guess this is exactly the reason why pretty-ts-errors exists, to bridge this gap and output errors that are easier to understand. If the errors on tsserver were better formatted, then we wouldn't need any of those packages 😄

I'm not sure if we are on the same page. The typescript-language-server is not tsserver. It's a third-party LSP implementation that utilizes tsserver. And it makes sense for it to integrate this project as long as tsserver doesn't do that itself.

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).

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.

@hexh250786313
Copy link

hexh250786313 commented Mar 7, 2024

@GabeDuarteM

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).

In fact, my coc-pretty-ts-errors supports two modes: one is the one you mentioned, which displays in the diagnostic panel, and the other is similar to the original pretty-ts-errors, which is displayed through the hover function. Displaying in the diagnostic panel is actually just an additional feature. Personally, I prefer to display it on hover, which is why I did not filter the original diagnostic information. I do not want to change anything from the language server; such filtering functionality should be implemented in the language server rather than by the plugin. coc-pretty-ts-errors simply follows the practice of pretty-ts-errors as a VSC plugin. In fact, since the coc.nvim plugin system is a variant of the VSC plugin system, the implementation of coc-pretty-ts-errors is similar to pretty-ts-errors in many aspects. Therefore, I did not consider too much whether my coc-pretty-ts-errors should be a plugin, an LSP, or something else. coc-pretty-ts-errors is just a "replica" of pretty-ts-errors.

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.

As for pretty-ts-errors-markdown, it is essentially just a formatting tool. You can do anything based on it, including implementing the LSP features you mentioned, or even making it a part of the original pretty-ts-errors. It does not contain any specific 'editor philosophy'.

On the PR you mentioned, the package is supposed to be a completely separate thing that replaces the HTML rendering that the original package provides with Markdown ones (which I believe is not completely supported by neovim as well, as shown on the screenshot above).

coc.nvim did not support markdown syntax highlighting in the diagnostic panel before, but I later created a PR to add this feature. As far as I know, the diagnostic panel of builtin-lsp indeed cannot support markdown highlighting. Therefore, I would recommend using the hover feature to integrate pretty-ts-errors. I am not certain about builtin-lsp's support for markdown in hover tooltips, but I believe that any client that has implemented LSP should be able to support markdown highlighting in hover tooltips to some extent, because the information display in hover is inherently designed to support filetype.

@hexh250786313
Copy link

But not sure if the author of this one wants to maintain a markdown variant in addition to the original one.

I will continue to port new changes from the original until it has its own markdown version.

@GabeDuarteM
Copy link
Author

GabeDuarteM commented Mar 10, 2024

@hexh250786313 hey, thank you for your input as well! It's nice to see some traction on this issue 😃

Personally, I prefer to display it on hover, which is why I did not filter the original diagnostic information. I do not want to change anything from the language server; such filtering functionality should be implemented in the language server rather than by the plugin.

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

As for pretty-ts-errors-markdown, it is essentially just a formatting tool. You can do anything based on it, including implementing the LSP features you mentioned, or even making it a part of the original pretty-ts-errors. It does not contain any specific 'editor philosophy'.

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...

As far as I know, the diagnostic panel of builtin-lsp indeed cannot support markdown highlighting. Therefore, I would recommend using the hover feature to integrate pretty-ts-errors. I am not certain about builtin-lsp's support for markdown in hover tooltips, but I believe that any client that has implemented LSP should be able to support markdown highlighting in hover tooltips to some extent, because the information display in hover is inherently designed to support filetype.

Thank you for the tip, depending on how this PR gets handled I can also explore the hover route, although, at least from my usage of nvim, I usually only see diagnostics through the normal diagnostics panel (that gets shown on hover, as seen on the screenshot on the first post), that way I can see all of them in a single view (eslint, typescript, etc). While I do prefer a lot more having the markdown view, I personally feel like it would be a brittle experience to separate typescript-specific diagnostics under a different keybind than everything else, which is why I believe that supporting the native diagnostics is probably the best move (although I don't mind the option to also allow the custom markdown view)


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

@CestDiego
Copy link

I would very much like the idea of opening this as an LSP such that other editors like Emacs and (neo)Vim can use it.

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 this pull request may close these issues.

Neovim support
4 participants