Add workspace/didChangeConfiguration
#528
Conversation
| if (!initializationOptions.inlayHints?.enable) { | ||
| return { | ||
| jsonrpc: c.jsonrpcVersion, | ||
| id: msg.id, | ||
| result: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
To clear inlay hints when setting is disabled.
There was a problem hiding this comment.
Doing this, and not restarting the server, means the client will continue to send messages to the server for these features. The point of turning the features off via a setting is also that they should be zero cost and not potentially slow down the extension. Even if this is just a null response, it'll still keep sending all of the messages back and forth. Might not hurt performance for inlay hints, but feature X, Y and Z that we'll add in the future might. Better to just turn it off completely via capabilities and a restart, like we're doing now. Then it's truly zero cost.
| if (!initializationOptions.codeLens?.enable) { | ||
| return { | ||
| jsonrpc: c.jsonrpcVersion, | ||
| id: msg.id, | ||
| result: null, | ||
| }; | ||
| } | ||
|
|
There was a problem hiding this comment.
To clear codelens when setting is disabled
| @@ -107,7 +107,7 @@ You'll find all ReScript specific settings under the scope `rescript.settings`. | |||
| | Prompt to Start Build | If there's no ReScript build running already in the opened project, the extension will prompt you and ask if you want to start a build automatically. You can turn off this automatic prompt via the setting `rescript.settings.askToStartBuild`. | | |||
| | ReScript Binary Path | The extension will look for the existence of a `/node_modules/.bin/rescript` file and use its directory as the `binaryPath`. If it does not find it at the project root (which is where the nearest `bsconfig.json` resides), it goes up folders in the filesystem recursively until it either finds it (often the case in monorepos) or hits the top level. To override this lookup process, the path can be configured explicitly using the setting `rescript.settings.binaryPath` | | |||
| | Inlay Hints (experimental) | This allows an editor to place annotations inline with text to display type hints. Enable using `rescript.settings.inlayHints.enable: true` | | |||
| | Code Lens (experimental) | This tells the editor to add code lenses to function definitions, showing its full type above the definition. Enable using `rescript.settings.codeLens: true` | | |||
| | Code Lens (experimental) | This tells the editor to add code lenses to function definitions, showing its full type above the definition. Enable using `rescript.settings.codeLens.enable: true` | | |||
There was a problem hiding this comment.
The setting in package.json is rescript.settings.codeLens.enable
| codeLens: { | ||
| enable: boolean; | ||
| }; |
There was a problem hiding this comment.
The setting in package.json is codeLens.enable
There was a problem hiding this comment.
To be frank, I don't see the point of this. Unless I'm mistaken it also subtly breaks a number of things.
Maybe you could elaborate a bit more on why this is necessary, and what the trade offs are? It's typically much easier to review a PR that has a clearly stated goal and some reflection on trade offs, especially when it does seemingly "deep" refactors and changes the behavior behind the scenes, like this does.
| initializationOptions: { | ||
| extensionConfiguration: workspace.getConfiguration("rescript.settings"), | ||
| }, | ||
| initializationOptions: workspace.getConfiguration("rescript.settings"), |
There was a problem hiding this comment.
While I appreciate wanting to keep thing simple, it's a choice whether we want to break user's configuration now or later. What if we decide we want more things sent to the server from the client at a later stage? Could be as simple as meta data around what client it is, or whatever. If we make this change, we'd need to migrate back to the current solution again to support that, and that'd break all clients. I think the current approach is more future proof, even if it means sending an extra prop.
| ) { | ||
| commands.executeCommand("rescript-vscode.restart_language_server"); |
There was a problem hiding this comment.
Did you test this in an actual editor? Since this affects what capabilities the LS itself is initiated with, I can't imagine turning features on still works after this change, since you'd need to update the capabilities, and to do that you must restart the server.
| client.sendNotification( | ||
| "workspace/didChangeConfiguration", | ||
| { settings: workspace.getConfiguration("rescript.settings") }).catch((err) => { window.showErrorMessage(String(err)) }) |
There was a problem hiding this comment.
While I do like this touch of notifying the server on config changes, this in combination with removing the pulling breaks all other clients than VSCode, unless they also implement this pushing. I'd rather pull and just have it work on all clients without the extra work needed.
| if (!initializationOptions.inlayHints?.enable) { | ||
| return { | ||
| jsonrpc: c.jsonrpcVersion, | ||
| id: msg.id, | ||
| result: null, | ||
| }; | ||
| } |
There was a problem hiding this comment.
Doing this, and not restarting the server, means the client will continue to send messages to the server for these features. The point of turning the features off via a setting is also that they should be zero cost and not potentially slow down the extension. Even if this is just a null response, it'll still keep sending all of the messages back and forth. Might not hurt performance for inlay hints, but feature X, Y and Z that we'll add in the future might. Better to just turn it off completely via capabilities and a restart, like we're doing now. Then it's truly zero cost.
Change
rescript.settings. Just send a notification and update the settingsPolish
initializationOptions. This makes setup easier on other clients like Neovimrequire'lspconfig'.rescriptls.setup = { cmd = { 'node', '/home/pedro/Desktop/Projects/rescript-vscode/server/out/server.js', '--stdio', }, init_options = { - extensionConfiguration = { - askToStartBuild = false - }, + askToStartBuild = false }, }