Declare workspaceFolders support in server capabilities #4666
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The `vscode-languageserver-node` dependency will attempt dynamic capability registration for workspace folder change notifications if this `workspace.workspaceFolders.changeNotifications` is not set in the `InitializeResult`. That is problematic for clients that don't support dynamic capability registration: if we send a JSONRPC error, the server will exit because of the rejected promise. The server has support for workspace folders and requests and uses change notifications as far as I can see, so I believe it's more correct to declare support for these up-front anyways.
|
Thanks for the contribution. Looks good to me. @heejaechang, I'm adding you as a reviewer also. |
erictraut
approved these changes
Feb 20, 2023
|
@heejaechang, any thoughts on this PR? If I don't hear from you in the next day, I'll assume it's OK and merge it. |
heejaechang
approved these changes
Feb 24, 2023
|
I think it is right thing to do. we already listen for workspace folder change event and support workspace folders. |
|
It looks like when we register for workspace folder change event, LSP library automatically send registerCapabilities for workspace folder change support. and that's why it worked fine for vscode and we didn't notice. |
This was referenced Mar 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
vscode-languageserver-nodedependency will attempt dynamic capability registration for workspace folder change notifications if thisworkspace.workspaceFolders.changeNotificationsis not set in theInitializeResult. That is problematic for clients that don't support dynamic capability registration: if we send a JSONRPC error, the server will exit because of the rejected promise.The server has support for workspace folders and requests and uses change notifications as far as I can see, so I believe it's more correct to declare support for these up-front anyways.
This is the recommended way to avoid dynamic capability registration for this case with
vscode-languageserver-node: (comment)Fixes #2690
Fixes #1302
This and a change within Helix (helix-editor/helix#6058) will resolve issues we started seeing in 1.1.293 (#4602)
See also microsoft/vscode-languageserver-node@5bc82ac
Revelant part of the spec: https://microsoft.github.io/language-server-protocol/specifications/lsp/3.17/specification/#workspace_workspaceFolders
Here is where the change notification handler is set up in pyright and what triggers dynamic capability registration within vscode-languageserver-node.