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

Remove server side document selectors #3074

Merged
merged 1 commit into from
Jan 17, 2025

Conversation

vinistock
Copy link
Member

@vinistock vinistock commented Jan 17, 2025

Motivation

I finally understood the mysterious requests we were receiving from URIs we didn't care about or the duplicate requests thanks to microsoft/vscode-languageserver-node#1487 (comment).

We were overriding the document selectors we define in the client in the server, with a much more simplistic approach. In the client, we take steps to ensure that we're handling only certain schemes with specific patterns and not duplicating client handling for the untitled scheme.

All of that configuration was lost for many requests because the server was simply broadcasting the document selector as { language: "ruby" }, which is not scoped to any scheme or pattern.

This results in every language server running in the current VS Code window advertise that they handle any Ruby file, which is why we kept receiving duplicate requests and why we kept receiving requests for schemes we are not interested in handling - like the vscode-chat-code-block:// scheme.

Implementation

Since our document selector logic is so complex and requires knowledge of default gem paths, bundled gem paths, workspace paths and so on, we should not override it from the server. Trying to implement the same document selector logic on both ends would probably lead to unnecessary headache.

All of our requests already specify in server.rb if they need to return early for a certain document type, so there's no risk of receiving undesired requests that we don't know how to handle.

Considering that, I just set all document selectors in requests to nil, which makes the editor prefer the one configured by the client.

Copy link
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@vinistock vinistock added server This pull request should be included in the server gem's release notes bugfix This PR will fix an existing bug labels Jan 17, 2025 — with Graphite App
@vinistock vinistock requested review from andyw8 and st0012 January 17, 2025 14:17
@vinistock vinistock marked this pull request as ready for review January 17, 2025 14:17
@vinistock vinistock requested a review from a team as a code owner January 17, 2025 14:17
Copy link
Contributor

@andyw8 andyw8 left a comment

Choose a reason for hiding this comment

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

Worth asking someone using an editor other than VS Code to help verify?

@vinistock vinistock merged commit b28710d into main Jan 17, 2025
44 checks passed
Copy link
Member Author

Merge activity

  • Jan 17, 10:05 AM EST: A user merged this pull request with Graphite.

@vinistock vinistock deleted the 01-17-remove_server_side_document_selectors branch January 17, 2025 15:05
@vinistock
Copy link
Member Author

Worth asking someone using an editor other than VS Code to help verify?

In other editors, you usually specify the document selectors are part of configuring the language server. For example, in NeoVim: https://github.com/neovim/nvim-lspconfig/blob/339ccc81e08793c3af9b83882a6ebd90c9cc0d3b/lua/lspconfig/configs/ruby_lsp.lua#L6.

The user has more control over the document selectors. It's mostly for VS Code that it gets hidden away in the client implementation. So now the server will respect whatever people configure in other editors.

Did you have a particular editor in mind for testing?

@andyw8
Copy link
Contributor

andyw8 commented Jan 17, 2025

Probably Neovim since it's seems most popular after VS Code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This PR will fix an existing bug server This pull request should be included in the server gem's release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants