[DO NOT MERGE] Global LS Command Registry to register/unregister and execute commands #215
Conversation
There was a problem hiding this comment.
Overall this looks good to me, only real concern is the singleton instance of the command registry. I like how you added the support for capability registration in a way that isn't fully exposed for everything yet, lets us open that up more broadly later when we're ready.
| } | ||
|
|
||
| private getLspCommandRegistry(): LspCommandRegistry { | ||
| if (!GLOBAL.lspCommandRegistry) { |
There was a problem hiding this comment.
Shouldn't this be per-server? Is there an advantage to making it available globally to all instances of atom-languageclient?
There was a problem hiding this comment.
There is an advantage to it. If this registry is global then it is possible to make language servers communicate between each other. The communication to JDT LS is great for the case of Spring Tools because it's mostly an add-on over the Java tooling. Information spring tools may want from JDT LS is classpath, search, javadoc etc.
Example of communication between Spring Tools LS and JDT LS
I have extended LSP with spring/javadoc request message with java artifact binding key and project uri as parameters for the message.
The message is received on my Atom language client extension.
Access global command registry and look for command with id jdt/javadoc for example which would be the command registered by JDT LS (ide-java package)
Execute the command and pass the binding key and project URI as parameters to the jdt/javadoc command
The command execution goes to the JDT LS and comes back with the result via LSP messages
The javadoc content is extracted from the result of the command execution and sent as a reply to the initial spring/javadoc command
Thus, I have javadoc in my spring tools LS from JDT LS at the end of this
There is a related PR for the ide-java package that uses the global registry: atom/ide-java#79
Nevertheless, I understand your concern with the global registry... It also introduces new API to work with the registry... I wish there was something in Atom core that could be utilized for this purpose.
Fixes #212
Introduces global LS commands registry where commands are registered. Registry is shared between all LS clients and can be accessed to execute commands.
AtomEnvironmenthasCommandRegistrybut it's unclear how to utilize it for LS client purposes... Seems like commands in AtomCommandRegistryare mainly for the UI (i.e. click or pick command in the command palette) since they don't take parameters...Don't think this is the final solution for the problem, but maybe it's a starting point to gather some feedback and turn this into a shape of a PR that can be merged.