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

[WIP] Introduce LSP mode for TSServer with some initial functionality #45347

Draft
wants to merge 40 commits into
base: feature/lspSession
Choose a base branch
from

Conversation

uniqueiniquity
Copy link
Contributor

@uniqueiniquity uniqueiniquity commented Aug 6, 2021

First step towards fixing #39459.
This PR targets the feature/lspSession branch, since we will likely want to implement the full LSP mode over the course of several PRs.

Here, we introduce a new command-line flag --useLsp for TSServer that allows it to communicate with a client via the Language Server Protocol.

To do so, we introduce two new projects: one in the src/jsonrpc folder that implements reading and writing of the JSON-RPC protocol over Node streams (based on the implementation of vscode-jsonrpc), and the other in the src/uri folder that implements parsing and formation of standard URIs (based on the implementation of vscode-uri).

Given these new utilities, we are able to implement a new subclass of NodeSession called LSPSession. In contrast with our existing IOSession, which uses our own custom message protocol, LSPSession uses the Language Server Protocol to communicate with a consuming client. When a message is received and its contents parsed into objects of LSP spec types (defined in src/server/lspProtocol.ts), it is handled by the corresponding method handler defined in src/server/lspHandlers.ts.
Note that when messages are received, they are explicitly queued as part of the Node.js poll phase (where I/O is handled), then processed as part of the Node.js check phase (where setImmediate callbacks are invoked). This allows LSP cancellation messages to be received in order to cancel requests that have not yet been handled.

At this stage, we have implemented handlers for the following LSP messages:

  • initialize
  • initialized
  • shutdown
  • exit
  • textDocument/didOpen
  • textDocument/didChange
  • textDocument/didClose
  • textDocument/hover
  • textDocument/signatureHelp

To validate the changes made to the Session class and the implementations of the LSP handlers, we have implemented a TestLspSession class (to be compared with the existing TestSession) that can be used to write unit tests for LSP functionality. Such tests are provided here for the hover and signature help features in the src/testRunner/unittests/tsserver/lsp folder.

@@ -11,14 +11,18 @@
"references": [
{ "path": "../compiler" },
{ "path": "../jsTyping" },
{ "path": "../services" }
{ "path": "../services" },
{ "path": "../uri", "prepend": true }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is prepend needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may not be 😅

@rchl
Copy link
Contributor

rchl commented Aug 12, 2021

To do so, we introduce two new projects: one in the src/jsonrpc folder that implements reading and writing of the JSON-RPC protocol over Node streams (based on the implementation of vscode-jsonrpc), and the other in the src/uri folder that implements parsing and formation of standard URIs (based on the implementation of vscode-uri).

I wonder why it was necessary to re-implement those. Despite the name, those packages are used by most if not all JS/TS-based language servers and are compatible with any editor.

EDIT: Looked at the changes and vscode-uri is actually used.

/**
* Predefined error codes.
*/
export namespace ErrorCodes {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A small question, why not enum?

@Spoutnik97
Copy link

Is there a plan to merge this ?

@amcasey
Copy link
Member

amcasey commented May 28, 2024

Ben and I have both been off the team for several years, but I think @jakebailey might be looking into this?

@jakebailey
Copy link
Member

Yes, it's on my list, though I don't think this PR itself will be a part of it directly.

@Spoutnik97
Copy link

Do you already have PR to follow your work?

I am a Zed user, and typescript lsp is very slow versus tsserver...

@jakebailey
Copy link
Member

No, not yet. But that LSP wrapper is more or less the same as tsserver so not sure why you'd have a performance difference.

@Spoutnik97
Copy link

This is what this issue seems to think : zed-industries/zed#5166

@brunogrcsada
Copy link

I think there's probably some crossed wires there - other editors like Helix which do use the LSP wrapper don't seem to have the same massive delay on returning results.

It definitely seems to be a lot more prominent in larger monorepos, which makes me think it's related to an IO issue

Sure, cleaner LSP bindings that are supported directly by the team would be great, but I don't see why there would be such a big disparity compared to tsserver

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants