Navigation Menu

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

Refactor for Server Initialization Options #1916

Merged
merged 7 commits into from Sep 30, 2022
Merged

Refactor for Server Initialization Options #1916

merged 7 commits into from Sep 30, 2022

Conversation

johnsoncodehk
Copy link
Member

@johnsoncodehk johnsoncodehk commented Sep 30, 2022

This PR refactors the initializationOptions interface for language server, and make language clients based on the v0.x version no longer available.

For language clients that do not support multiple servers, just need to change typescript.serverPath, typescript.localizedPath to typescript.tsdk.

initializationOptions = {
	typescript: {
-		serverPath: '/usr/local/lib/node_modules/typescript/lib/tsserverlibrary.js'
-		localizedPath: '/usr/local/lib/node_modules/typescript/lib/ja/diagnosticMessages.generated.json'
+		tsdk: '/usr/local/lib/node_modules/typescript/lib'
	}
}

For multiple servers supports, we don't have languageFeatures and documentFeatures anymore, and you need to modify language client capabilities of InitializeParams for hint to language server which features should not be active.

But server diagnostic is use push model by default and do not care about client capabilities, to disable diagnostic, you can setup { serverMode: 2 } to create language server that only active syntactic features, or config { diagnosticModel: 0 } to disable diagnostic for semantic features server.

You can refer to:

cc @yaegassy, @sethidden, @tsukkee, @predragnikolic, @rchl, @kabiaa, @jadestrong, @tommasongr, @yoyo930021, @xiaoxin-sky

@johnsoncodehk johnsoncodehk merged commit a75fdf2 into master Sep 30, 2022
@johnsoncodehk johnsoncodehk deleted the server branch September 30, 2022 11:06
@rchl
Copy link
Collaborator

rchl commented Sep 30, 2022

Just curious... did you consider having a single Volar server (process) and have it start additional processes, if needed, and route requests accordingly?

That would be quite convenient for the clients (at least for ST) since this approach of starting multiple servers is not supported very well.

Though I see that you are not running typescript server in a separate process so that's probably why you are not doing that. It would all be blocking the main process...

@johnsoncodehk
Copy link
Member Author

johnsoncodehk commented Sep 30, 2022

This is an interesting idea. Maybe there could be a generic routing language server to support multiple servers for any languages.

But honestly multiple servers are not the ultimate solution and because it's increase memory usage with TS language service in large project, a better way is optimize IDE request strategy to mitigate the impact of request blocking, usually the biggest trouble is that slow diagnostic semanticTokens requests blocking next autocomplete request, I think IDE can record the language server diagnostic semanticTokens time and change the diagnostic semanticTokens request interval dynamically to reduce chance of blocking autocomplete.
(Edit: diagnostic is use push model by default and should be controllable by language server)

An another major problem is LSP cancel token is useless for TS, because the TS API is synchronous and the cancel token cannot receive updates from the LSP until the TS API execution is complete.

If we can solve either of the above, I hope to avoid use multiple servers.

@rchl
Copy link
Collaborator

rchl commented Sep 30, 2022

An another major problem is LSP cancel token is useless for TS, because the TS API is synchronous and the cancel token cannot receive updates from the LSP until the TS API execution is complete.

But I guess that's the consequence of Volar using Typescript API synchronously. It could spin up a separate process for it like typescript-language-server does and communicate through IPC/stdio. Then it wouldn't be blocking the main process.

@johnsoncodehk
Copy link
Member Author

johnsoncodehk commented Sep 30, 2022

I do want to migrate to tsserver, I'm still waiting for microsoft/TypeScript#47600, but I found it is just closed...

@johnsoncodehk
Copy link
Member Author

I just found how TS supports cancellation token for tsserver (https://github.com/Microsoft/TypeScript/wiki/Standalone-Server-%28tsserver%29#cancellation, https://github.com/microsoft/TypeScript/blob/663b19fe4a7c4d4ddaa61aedadd28da06acd27b6/src/cancellationToken/cancellationToken.ts#L11-L20), and implement in 2e1108c. Now we're down from 3 servers to 2 servers.

We have a new VueServerInitializationOptions#cancellationPipeName option, this is for implement server cancellation token that similar to tsserver. For support it, language client need to write a tmp file when editor editing vue document (or also TS document if enable takeover mode) and pass the file name to cancellationPipeName, language server will check the mtime (modify time) of the tmp file to know if language server should cancel current request working in synchronous API.

@rchl
Copy link
Collaborator

rchl commented Oct 2, 2022

Why does client have to do it rather than the language server creating a temporary, random file?

@rchl
Copy link
Collaborator

rchl commented Oct 2, 2022

Sorry, didn't look at your code carefully and didn't noticed that the client is supposed to write to the file also on file changes.

I must say that I don't like pushing this responsibility to the client but I'm guessing it's optional so it should be fine not to implement it.

@johnsoncodehk
Copy link
Member Author

This is optional not requirement, at least for now it can only be implemented on the client side.

I must say that I don't like pushing this responsibility to the client but I'm guessing it's optional so it should be fine not to implement it.

Yes, here is just to provide a solution, it is up to the client whether to use it or not. Actually typescript-language-server also has this, but you may not use it in client yet.

@rchl
Copy link
Collaborator

rchl commented Oct 2, 2022

typescript-language-server creates the file itself. It doesn't need to come from the client. But it makes sense since it doesn't use typescript API synchronously so it isn't blocked by it.

@johnsoncodehk
Copy link
Member Author

Sorry i didn't make it clear, The "blocking" mean in my words is that outdated requests cannot be cancel while executing a synchronous API, thus blocking new incoming requests, not mean blocking the main process. If client do not implement cancellationPipeName typescript-language-server also has same problem.

You can reference to VSCode for how it implement and provide cancellationPipeName to tsserver: https://github.com/microsoft/vscode/blob/61f8dee571efac71035000f4c57f1c42ada04e46/extensions/typescript-language-features/src/tsServer/cancellation.electron.ts#L27

@rchl
Copy link
Collaborator

rchl commented Oct 2, 2022

typescript-language-server does use cancellation tokens for some requests. For example for requesting diagnostics here: https://github.com/typescript-language-server/typescript-language-server/blob/7f69c27eb8cce71d3db006623757a74f93d76dd3/src/lsp-server.ts#L330-L337

And the client doesn't need to do anything because the "cancellationPipeName" is created by the server and is utilized whenever client sends a standard LSP requests (though most requests don't care about canceling old requests currently).

Since typescript-language-server is using typescript through the IPC or STDIO, it is not being blocked itself and can write to that cancellation file itself.

@johnsoncodehk
Copy link
Member Author

Thanks for the clarification. I can see that typescript-language-server seems is a LSP server version of typescript-language-features.

@rchl
Copy link
Collaborator

rchl commented Oct 2, 2022

Yes, exactly.

@mikehaertl
Copy link

@johnsoncodehk With nvim-lspconfig now certain features like goto definition or hover don' work anymore. I assume it has to do with this change.

For multiple servers supports, we don't have languageFeatures and documentFeatures anymore, and you need to modify language client capabilities of InitializeParams for hint to language server which features should not be active.

But server diagnostic is use push model by default and do not care about client capabilities, to disable diagnostic, you can setup { serverMode: 2 } to create language server that only active syntactic features, or config { diagnosticModel: 0 } to disable diagnostic for semantic features server.

I have problems to follow this.

If I understand right you ignore client capabilities now? Your explanation sounds like you only need extra configuration if you want to turn capabilities off - but that's not what I found.

Can you explain a bit more what configuration we need to enable these capabilites again?

@johnsoncodehk
Copy link
Member Author

johnsoncodehk commented Nov 13, 2022

@mikehaertl It should be resolved by 8daa162

@mikehaertl
Copy link

@johnsoncodehk Cool, thanks. Looking forward to the next release so I can test it.

@mikehaertl
Copy link

@johnsoncodehk I finally compiled vue-language-server.js from the latest master and it really seems to fix the issue.

Would you consider creating a 1.0.10 release to make this easier to install? There's quite some people affected by this bug and thay all can't update to 1.x due to this problem.

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.

None yet

3 participants