Single LanguageClient #834
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Refactors ClientHandler to only use one `LanguageClient` per extension activation. Removes the Map of `LanguageClients` and ensures that any attempt to use a `LanguageClient` or method happens after the onReady event finishes. Previously the VS Code extension would create a `LanguageClient` for each folder that was added to the current workspace. Each `LanguageClient` would handle all events and receive notifications/requests from the LS for the folders it was responsible for. This was handled by a `Map` of clients created at activation and populated each time a client was created. However there were conditions in which an event would occur before a client was created or finished being ready, so an attempt to access the `Map` would result in an unhandled exception. A common example was having an file that was already opened when VS Code was opened. The file would trigger an `onDidChangeVisibleTextEditors` event, which would try to get the list of module calls used, which would fail because the client had not started yet. The terraform-ls sever implemented multi-folder/workspace support in hashicorp/terraform-ls#502. This means the VS Code extension no longer needs multiple clients to track how many folders are added to the current workspace and which event for which file needs to be sent at certain times.
Moves the list of supported commands to a private property of the ClientHandler class and populates it when the client and LS are initialized. Previously you could attempt to ask the server what commands were supported before the client and server had finished the initialization process. This would result in an unhandled exception and leave the client in an unknown state. Moving the query to the `onReady` function ensures we only ask after the server has finished initializing and is ready to respond.
Refactor the createTerraformClient method to reduce complexity and verbosity by using language constructs. Move variables closer to usage for future method extraction. In d707a6d we moved to a single LanguageClient in the extension. This means we no longer need to track the `location` or `workspaceFolder` to pull settings or display information per folder. This greatly simplifies the code accessing settings and creating the LanguageClient.
Extract `resolvePathToBinary` method to the ServerPath class. The `resolvedPathToBinary` resolves the LS filesystem path and uses methods inside the ServerPath class. The ClientHandler class is only responsible for LanguageClients, and so does not need this private method.
Only enable the ModuleProvider if the LS is enabled, as it requires functionality from the LS to operate.
Directly call `updateLanguageServer` in the main execution flow and await return instead of inside a `executeCommand` call. The `terraform.enableLanguageServer` command checks if the LS is enabled, then returns `updateLanguageServer`. We already checked if the LS is enabled before calling `terraform.enableLanguageServer`, so we are duplicating effort here. There is also no reason to nest the async calls to the main functionality of our extension inside a `executeCommand` call. Technically `executeCommand` calls are supposed to be commands that initiate actions/commands/things inside the workspace, not long running operations. This does not match well with running the primary execution work flow of our extension.
Move creation of the OutputChannel to the main flow and only create one outputChannel for the entire extension. Use 'HashiCorp Terraform' as the name the OutputChannel to brand our extension. The OutputChannel is the main way to log information for the user to see. By convention there is one for each extension and the content is meant to be human readable. The `name` is displayed in a drop down in the `Output` pane in the editor, so it should be reconizable and readable. Currently we set `terraform-ls` or `terraform-ls 'location'` for the name. This lower cased name is hard to find in the drop down, espcially compared to other extensions that use their company name and language to disambiguate from others. Setting the name to `HashiCorp Terraform` ensures we are easy to read and differentiate from other extensions. Creating only one for the entire extension makes it easier for users to find out information as well as allowing us to automate collecting logs for bug report submission.
jpogran
force-pushed
the
single-language-client
branch
from
aa878d2 to
ebc76b4
Compare
dbanck
reviewed
Nov 5, 2021
| } | ||
|
|
||
| const experimentalFeatures = config('terraform-ls').get('experimentalFeatures'); | ||
| const initializationOptions = Object.assign( |
There was a problem hiding this comment.
I was wondering if something like this is more readable:
const initializationOptions = {
...experimentalFeatures,
...(terraformExecPath.length > 0 && { terraformExecPath }),
...(terraformExecTimeout.length > 0 && { terraformExecTimeout }),
...
};
jpogran
force-pushed
the
single-language-client
branch
from
6e4cd98 to
49ddf6f
Compare
jpogran
force-pushed
the
single-language-client
branch
from
49ddf6f to
69f59d9
Compare
|
Superseded by #845 |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Close #816
Relies on test fixes in #828