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 configuration aspect from onInitialConfiguration? #253

Closed
lukel97 opened this issue Aug 28, 2020 · 7 comments
Closed

Remove configuration aspect from onInitialConfiguration? #253

lukel97 opened this issue Aug 28, 2020 · 7 comments

Comments

@lukel97
Copy link
Collaborator

lukel97 commented Aug 28, 2020

A lot of servers seem to be using initializationOptions in the initialize request to pass the workspace configuration in, but see the discussion here microsoft/language-server-protocol#567 and the official recommendation here:
microsoft/language-server-protocol@545cd69

It looks like they are indeed separate things, and that initializationOptions is more for command line arguments etc. However our API currently suggests that they should be treated the same, by providing a callback to extract the workspace configuration from initializationOptions and store it in the resConfig state.

data InitializeCallbacks config =
  InitializeCallbacks
    { onInitialConfiguration :: InitializeRequest -> Either T.Text config

To me it seems like the resConfig state should be used to keep track of the most recent workspace/didChangeConfiguration notifications exclusively (and we should extend this to be updated whenever a workspace/configuration request is made from the server).
Should we change onInitialConfiguration to something more like:

data InitializeCallbacks config =
  InitializeCallbacks
    { onInitialize :: InitializeRequest -> LspM config ()

to guide server authors down the right path. This LspM monad is from the type safe branch in #244. If authors still need to get the workspace configuration immediately after initialization, then in LspM they can make a workspace/configuration request to pull it from the client.

@lukel97
Copy link
Collaborator Author

lukel97 commented Aug 28, 2020

cc @alanz @wz1000

@jneira
Copy link
Member

jneira commented Aug 28, 2020

Agree, when working in the config acess in Rules i did not see in the protocol docs nothing about two sources of config should be the same (both are Any 😜 ) so i think they should be separated in our code. Even if they are the same at the end, for some server implementations.
I've observed that hls/ghcide cant generate a correct config with the initialization options and always is Nothing. vscode client triggers a didConfigurationChange just after the server is started so the config is always initialized that way.

@lukel97
Copy link
Collaborator Author

lukel97 commented Aug 28, 2020

If vscode sends a didConfigurationChange on start-up then that should probably the norm that other clients follow too

@lukel97
Copy link
Collaborator Author

lukel97 commented Aug 28, 2020

What I have so far looks like:

-- | Contains all the callbacks to use for initialized the language server.
-- it is parameterized over a config type variable representing the type for the
-- specific configuration data the language server needs to use.
data InitializeCallbacks config =
  InitializeCallbacks
    { onConfigurationChange :: J.Value -> LspM config (Either T.Text config)
      -- ^ @onConfigurationChange newConfig@ is called whenever the
      -- clients sends a message with a changed client configuration. This
      -- callback should return either the parsed configuration data or an error
      -- indicating what went wrong. The parsed configuration object will be
      -- stored internally and can be accessed via 'config'.
    , onInitialization :: InitializeRequest -> LspM config (Maybe ResponseError)
      -- ^ Called after receiving the initialization request and before returning the response.
      -- This callback will be invoked to offer the language server
      -- implementation the chance to create any processes or start new threads
      -- that may be necesary for the server lifecycle.
      -- It can also return an error in the initialization if necessary.
    }

@alanz
Copy link
Collaborator

alanz commented Aug 28, 2020

see also #210 and #211

@alanz
Copy link
Collaborator

alanz commented Aug 28, 2020

The intention behind the init is you may be using it for one-time startup stuff. And there is no guarantee that they should be the same, so we should allow them to be different.

That said, command line args to launch the server are probably the best way to actually pass init options.

@wz1000
Copy link
Collaborator

wz1000 commented Oct 15, 2020

Fixed in #244

@wz1000 wz1000 closed this as completed Oct 15, 2020
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

4 participants