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

All servers should share Workspace Configuration #2243

Open
rchl opened this issue Apr 30, 2023 · 3 comments
Open

All servers should share Workspace Configuration #2243

rchl opened this issue Apr 30, 2023 · 3 comments

Comments

@rchl
Copy link
Member

rchl commented Apr 30, 2023

Is your feature request related to a problem? Please describe.

In VSCode (and maybe other editors) all servers have access to the whole editor configuration which means that one server can access configuration of another server and which is why server settings are normally scoped with a prefix of the server name or similar (for example json.* for LSP-json or javascript.*/typescript.* for LSP-typescript etc.).

That means that one server can reuse settings of another server. For example Volar, being a sort of a "meta server", can read settings of LSP-html, LSP-typescript, LSP-css, etc...

Describe the solution you'd like

Opening server-specific settings like Preferences: LSP-json Settings should expose settings object as the root object on the left and a global LSP-server-settings.sublime-settings on the right.

Question 1: Where to expose initializationSettings, selector, command and other settings in that case? In VSCode the extension can contribute extra settings that are then exposed.

Question 2: How to specify which settings should be sent to the extension on client-initialized workspace/didChangeConfiguration? In VSCode the extension does that with some extra code.

Question 3: Since settings of LSP-typescript would then apply for both .js, .ts and .vue files, one might want to specify different javascript.* settings for vue files so ideally we'd also support language-id specific overrides. In VSCode possible with something like:

    "[python]": {
        "editor.formatOnType": true
    },

Additional context

All in all this doesn't seem very feasible to do in ST but I've reported as a formality as this is how settings are handled in VSCode.

@rchl
Copy link
Member Author

rchl commented Apr 30, 2023

I suppose this would solve #2044 as we wouldn't have to restart the server knowing that only workspace configuration has changed.

Also I remember we've discussed that quite a bit but not sure where that was. Maybe in some PR. I remember @jwortmann has been experimenting with separating workspace settings.

@jwortmann
Copy link
Member

In VSCode (and maybe other editors) all servers have access to the whole editor configuration which means that one server can access configuration of another server and which is why server settings are normally scoped with a prefix of the server name or similar

The difference in VSCode is, that it uses only a single settings file for everything, i.e. global editor settings, language specific settings, and settings from extensions all in one file. This is why the prefix like json.* in the setting names exists, so that there is no risk of naming conflicts. The fact that language servers can access the whole editor configuration is a byproduct of that, and not the cause.

Another aspect is, that in VSCode the language servers are either already included in the VSCode installation (JSON, HTML, CSS, ...), or bundled in the language extension. In fact, the setting prefix in most cases represents the language name and not the name of the language server, for example python.analysis.extraPaths and not pyright.analysis.extraPaths for Pyright/Pylance. In other words, the language server just use the prefix of the extension, which usually is the language name.

Sublime Text on the other hand uses the convention of separate settings files per language or per extension package, so I think it was natural to include the language server settings in the settings file from the LSP-* helper package. But I can see the appeal to have only a single file for all server-specific settings (and I think I faintly remember a brief discussion about this somewhere in this repo a few years ago).

That means that one server can reuse settings of another server. For example Volar, being a sort of a "meta server", can read settings of LSP-html, LSP-typescript, LSP-css, etc...

I haven't used Volar, but I wonder how that works in practice. Do the Volar dev(s) actively cooperate with the other extension devs to agree on particular setting names? That seems to be error prone to me, if a language extension wants to introduce new or change existing settings names, which might get unnoticed by Volar. I see that in LSP-volar.sublime-settings, they use even settings with prefix emmet.* from the Emmet extension...

Functionality-wise, I also don't see a real advantage to the status quo. For example for Volar, you just put all settings into the LSP-volar.sublime-settings file, even if some of them use the typescript.* prefix and if the same setting is also used in LSP-typescript.


I think we can classify four different kind of settings used in LSP:

  1. global package settings, for example "show_view_status": true
  2. language server start configuration ("enabled", "command", "schemes", "selector", "initializationOptions", "disabled_capabilities", etc.) - those are predefined setting names used by the main LSP package
  3. LSP-* helper package settings, for example "show_environment_status" in LSP-julia - unknown to main LSP package
  4. language server settings (the "settings" object)

The language server only needs to be restarted, if what I called here "start configuration" (2) changes, and perhaps also if the helper package settings (3) changes (dependent on the implementation). If the language server settings (4) changes, then we need to send a ‘workspace/didChangeConfiguration’. If we want to implement that, they need to be put into separate files, so that we can watch for file change events via the Settings.add_on_change API. This should more or less answer

Question 1: Where to expose initializationSettings, selector, command and other settings in that case?

i.e. following your proposal of

Opening server-specific settings like Preferences: LSP-json Settings should expose settings object as the root object on the left and a global LSP-server-settings.sublime-settings on the right

we would need to have one file containing the language server start configuration (2) as well as the helper package settings (3), and another file containing the language server settings (4). I assume that the start configuration should still be possible to be modified by users, and to have a settings file for the potential package settings seems to be necessary, so I think the first file is still needed. A disadvantage of your proposal to put all user settings for (4) into a single file is, that this goes strictly against Sublime's convention that the values on the right side should directly overwrite (only) the values on the left side. A user could open the Preferences: LSP-json Settings and put his/her user settings on the right. Later they open the settings for another language server Preferences: LSP-typescript Settings, and be surprised that there is already some other "junk" (their LSP-json user settings) on the right side, which doesn't even match the predefined settings from LSP-typescript on the left. So they might just delete at that and only put the settings relevant to LSP-typescript on the right side now.

Question 2: How to specify which settings should be sent to the extension on client-initialized workspace/didChangeConfiguration?

While not optimal, I suppose it would be possible to just send everything to all servers. The prefixes should ensure that there are no naming conflicts, and that the unnecessary settings are ignored by the server.

Another option would be to send only the setting names which are exposed in the settings file from the LSP-* helper package. But this would only work if a LSP-* helper package exists, and also the settings specified therein must be complete (i.e. users are not able to specify additional settings which are not included in the predefined settings).


This is already too much text, so I'll quit it for now.

But I think it would be very awkward/difficult to implement it in a non-breaking way. So if the benefit of this outweights the drawback of breaking changes to users, I would probably still wait with making such a change. Maybe when/if LSP switches to Python 3.8, which would then require changes to all LSP-* packages anyway.

By the way, the other thread with a discussion about splitting the settings was in #2033 (comment).

@rchl
Copy link
Member Author

rchl commented May 1, 2023

I haven't used Volar, but I wonder how that works in practice. Do the Volar dev(s) actively cooperate with the other extension devs to agree on particular setting names? That seems to be error prone to me, if a language extension wants to introduce new or change existing settings names, which might get unnoticed by Volar. I see that in LSP-volar.sublime-settings, they use even settings with prefix emmet.* from the Emmet extension...

Volar basically bundles those VSCode language servers (emmet, json, html, css, ...) so it somewhat automatically supports the same settings as VSCode. In practice there is quite a bit of manual code added on top of integrating those so things are not always in sync.

Functionality-wise, I also don't see a real advantage to the status quo. For example for Volar, you just put all settings into the LSP-volar.sublime-settings file, even if some of them use the typescript.* prefix and if the same setting is also used in LSP-typescript.

I almost see one advantage in that we wouldn't have to duplicate settings and schema in two different packages. But then the user would have to install LSP-css to get relevant settings even if only needing support for CSS in Vue files through LSP-volar. So I guess I can't say that it's a realistic ask.

we would need to have one file containing the language server start configuration (2) as well as the helper package settings (3), and another file containing the language server settings (4).

In theory the helper package settings (3) could be merged into language server settings (4). In most cases servers should not mind extra settings (although I've seen some that do, at least if sharing same language prefix).
Then I guess the helper package would have to observe changes to the setting and decide whether server needs to be restarted or not.


All in all, this sounds like a way too disruptive change and not clearly better in all aspects than the current design so probably will never happen in practice. But thanks for discussing it. At least we have some issue to point to if this comes up again. :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants