Adding a command to stop the LSP server #36
Conversation
Signed-off-by: Maxim Zaks <maxim.zaks@gmail.com>
| this.pushSubscription( | ||
| vscode.commands.registerCommand('mojo.lsp.stop', async () => { | ||
| if (this.lspClient) { | ||
| await this.lspClient.stop(); |
There was a problem hiding this comment.
When we stop the LSP here 'this.lspClient' is still set. Later in the file when we watch for new mojo files being opened (line 226) we're checking to see if lspClient is set, and so leaving it set here would prevent the LSP being restarted when opening new mojo files.
The intent of this PR is to allow the user to stop the LSP and avoid any performance issues, so it makes sense that the user may not want it to restart - but I feel there's a usability question here: Does the user expect the LSP to stop permanently until manually restarted, or do they expect it to stop only for the currently open file?
If we are comfortable with it stopping until manually restarted I'm not sure keeping this.lspClient defined even after the LSP stops is necessarily the best way to go about it.
There was a problem hiding this comment.
I think stoping LSP until user restarts it again is the desired behavior, as users perform an explicit action for stopping and it can be very contra intuitive if this action is reverted implicitly by a different action (e.g. open a new Mojo file). I think explicit action should be revertible by another explicit action.
There was a problem hiding this comment.
Sorry for the delay, I've been implementing some CI improvements and fixing other issues so we can try to get a release out to include some of your improvements. It would be good to merge this one too.
I agree with you about explicit actions, though I am still not keen on leaving this.lspClient set after the lsp has been stopped, since other functionality (current and future) may check this value and get the wrong understanding of the LSP state. Two easy options could be:
- Add some new boolean called preventLSPStart or similar which we can set in the stop command, and test for in tryStartLanguageClient
- Accept this PR as is, but add an issue to the tracker making note of the situation as a low priority to fix
- Add a TODO comment in the new mojo.lsp.stop code pointing out that this.lspclient is still set
2 seems easiest. If you're in agreement let me know and update your branch, I'll merge it and make the issue. Otherwise feel free to implement the other options or suggest alternatives.
2 participants
This PR addresses following feature request #24