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

Changing the configuration after workspace/didChangeConfiguration requests #414

Closed
pgujjula opened this issue Mar 30, 2022 · 8 comments
Closed

Comments

@pgujjula
Copy link
Contributor

I notice that lsp now has a function getConfig to get the client configuration "as set via the initialize and workspace/didChangeConfiguration requests." I am a little confused how the configuration is supposed to be updated, however.

Our language server has an implementation of onConfigurationChange that parses the changed configuration. When we try changing the configuration, the lsp library complains with an error lsp:no handler for: SWorkspaceDidChangeConfiguration. I assume this means that we should also provide an implemention for the workspace/didChangeConfiguration notification. However, it's unclear to me what this handler should be doing, since it seems all the relevant work is already done by the onConfigurationChange function.

Therefore, my question is, what functionality should the onConfigurationChange implementation provide, what functionality should the workspace/didChangeConfiguration handler provide, and what is the difference between them?

@pgujjula
Copy link
Contributor Author

pgujjula commented Apr 7, 2022

I'd like to bump this question, @michaelpj perhaps?

@michaelpj
Copy link
Collaborator

What we currently do with onConfiguraitonChange is:

  1. If the intialize request contains initializationOptions, we call it on that (this is somewhat questionable but some clients expect it).
  2. We put it in the LanguageContextEnv.

So at the moment you need to add your own didChangeConfiguration notification handler, which may well just pull onConfigurationChange from the LanguageContextEnv and call it. I think the reason we don't install a handler by default is that a server may need to do other things in the didChangeConfiguration handler, e.g. invalidating state. So it's something we can't write for people.

@pgujjula
Copy link
Contributor Author

pgujjula commented Apr 11, 2022

Thanks for the information! I'll try implementing our configuration management using this info and get back to you if I have any more questions.

@pgujjula
Copy link
Contributor Author

pgujjula commented Apr 12, 2022

Ok, I've come across a problem with this setup. When the configuration is changed in VSCode, the client sends a workspace/didChangeConfiguration notification with a null body (see microsoft/vscode-languageserver-node#380 (comment)), and expects the server to re-read the configuration from the client.

When the server receives a workspace/didChangeConfiguration message, it seems that it calls the parser specified in onConfigurationChange to parse the config, updates the stored config, and also calls the user-specified handler for SWorkspaceDidChangeConfiguration.

The problem is that in this flow of logic, there is no way to ensure that the config is actually updated properly, if the client is sending a null notification. The onConfigurationChange parser is pure, so it when it's passed a null notification to parse, it can't request the actual config from the client. The user-specified handler for SWorkspaceDidChangeConfiguration can request the actual config, and it can call onConfigurationChange to parse the config, but it can't actually update the config in the server because there's no function in the lsp API to do so.

I think the easiest way to solve this issue would be to add a setConfig function to the lsp API, analogous to the getConfig function that is already present. The setConfig function would be called by the user-specified handler for SWorkspaceDidChangeConfiguration notifications. I can make a PR for this if this plan sounds good to you @michaelpj

@michaelpj
Copy link
Collaborator

Ah indeed I was wrong, we do handle this by default.

Having setConfig would be fine. I think this is a further case for unbundling the server implementation so it's easier to this stuff yourself...

@pgujjula
Copy link
Contributor Author

I created the PR here: #418

pgujjula added a commit to pgujjula/lsp that referenced this issue Apr 18, 2022
pgujjula added a commit to pgujjula/lsp that referenced this issue Apr 18, 2022
@pgujjula
Copy link
Contributor Author

Merging #418 solved this problem, so if there's no objections in a couple of days, I'll close this issue.

@michaelpj
Copy link
Collaborator

We now automatically call workspace/configuration when we get a didChangeConfiguration notification, so you should be able to remove your custom logic here. It's still annoying that you have to register an empty didChangeConfiguration handler. I have some thoughts about that: #520

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

No branches or pull requests

2 participants