Refactor to Single Language Client #845

jpogran · 2021-11-10T16:27:07Z

Refactor Language Client Workflow

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. The terraform-ls sever implemented multi-folder/workspace support in Support multiple folders natively 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.
Refactors the createTerraformClient method to reduce complexity and verbosity by using language constructs. Move variables closer to usage for future method extraction. 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.
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 name is displayed in a drop down in the Output pane in the editor, so it should be reconizable and readable.
Optimizes main execution flow by bringing language server updater to main execution flow, reducing the cases where a restart needs to occur to update, and reducing nesting constucts.
Extracts 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.

Testing

single folder workspace
multiple folder workspace
starting editor with no documents open
starting editor with one or more documents already open
starting editor with module view expanded

actions for each test:

open document, trigger intellisense and complete a resource from a provider
hover over a declaration or field and see hover doc info
navigate using any of the go to commands
navigate using variable reference

Closes #816

dbanck

Fantastic work on the refactoring! It's much easier to understand and follow what is happening on activate now. 🌟

I took some extra time for this review because I wanted to understand how everything works. So please don't feel discouraged by the number of comments. Everything is working well for me, and most are language-related things.

src/extension.ts

src/clientHandler.ts

radeksimko

Looks so much cleaner! ❤️ Thank you.

Just for clarity - Daniel already commented on the language aspect of the PR and my review is not meant to say my ✅ overrides his review 😄 but I trust the judgment of you two on this and so I'll focus on other aspects instead.

I have just a few questions, but none of them truly blocking:

If I'm understanding correctly, we now no longer support single-folder LS, which now no longer works with the latest extension anyway due to other incompatibilities. For transparency, "future-proofing" and just being a good citizen in LSP - how would you feel about raising an error when the server (for any reason) communicates that it doesn't support multiple folders? I'm not sure what exact implications does that have, but I'm assuming it may not work as expected in that case - either way it seems like we shouldn't just silently ignore such a (rare) case.

Use 'HashiCorp Terraform' as the name the OutputChannel to brand our extension.

I like the idea that we'd have a single pane to point users to... However I'm not sure we're ready for this (yet). 🤔

Maybe if the server used LSP-based logging via window/logMessage this would be viable, but as long as it's just stream of bytes there's a real risk that the two just get randomly mixed up. Even if/when we convert all the LS logging to the LSP method there's still some logs related to startup, before the RPC server is even ready and I'm not sure what the performance implications of potentially verbose logging over LSP are.

All that said the risks are currently pretty low as we already use a single pane and the extension itself doesn't log as much - so I'm 👌🏻 with this as-is, but I feel we need to talk about our logging strategy soon before the extension starts to utilize the log pane more.

In contrast the official Go extension registers 5 panes in total, which I'm not necessarily presenting as an example we should follow, but merely for context.

src/extension.ts

jpogran · 2021-11-12T14:20:24Z

If I'm understanding correctly, we now no longer support single-folder LS, which now no longer works with the latest extension anyway due to other incompatibilities. For transparency, "future-proofing" and just being a good citizen in LSP - how would you feel about raising an error when the server (for any reason) communicates that it doesn't support multiple folders? I'm not sure what exact implications does that have, but I'm assuming it may not work as expected in that case - either way it seems like we shouldn't just silently ignore such a (rare) case.

We check if LS supports multi-folder workspaces and log to the console currently, I could add a popup (that could be suppressed) notifying the user that the LS doesn't support the current workspace configuration.

Use 'HashiCorp Terraform' as the name the OutputChannel to brand our extension.

The 'branding' change was to align with how most other popular extensions name the channels, i.e. with human readable names. This is the opportunity to distinguish our logs from other extensions that could be installed at the same time. To me, I look at logs when something is going awry, and at that time it is more important to me to know which log is which extension so I can identify the next steps to take. I think we're in agreement that the branding makes sense and it's more the second comment that's the question, but just writing here to make sure.

Maybe if the server used LSP-based logging via window/logMessage this would be viable, but as long as it's just stream of bytes there's a real risk that the two just get randomly mixed up. Even if/when we convert all the LS logging to the LSP method there's still some logs related to startup, before the RPC server is even ready and I'm not sure what the performance implications of potentially verbose logging over LSP are.

We already log all LS to the outputchannel for that client, so not sure that there is any net new functionality here to think about throughput. The change here is that there is now only one: previously we created an outputchannel per client, since we only have one client now we only have one outputchannel. There is room for improvement in how/when we log, and as you state we'll be adding more to it to help users identify what is going on, but I think that is another PR.

radeksimko · 2021-11-12T14:40:00Z

I could add a popup (that could be suppressed) notifying the user that the LS doesn't support the current workspace configuration.

I think that would be nice.

We already log all LS to the outputchannel for that client, so not sure that there is any net new functionality here to think about throughput.

Yep, that's why I say it's totally fine in the context of this PR 😃 👍🏻 What seems to be changing is the (dev) perception as previously we'd call it terraform-ls and extension logs would end up there more by accident, now we more explicitly acknowledge that it's a shared output pane, so it's more tempting to send more logs there.

Again - not a big deal and certainly not a blocker for this PR, just something to think about more broadly.

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. 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. Refactors the createTerraformClient method to reduce complexity and verbosity by using language constructs. Move variables closer to usage for future method extraction. 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`. 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 `name` is displayed in a drop down in the `Output` pane in the editor, so it should be reconizable and readable. Optimizes main execution flow by bringing language server updater to main execution flow, reducing the cases where a restart needs to occur to update, and reducing nesting constucts. Extracts `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.

Co-authored-by: Daniel Banck <dbanck@users.noreply.github.com>

Co-authored-by: Radek Simko <radek.simko@gmail.com>

dbanck

Thank you for addressing and fixing all my comments. 👍

I found just three minor things which maybe got lost. Feel free to merge once addressed.

dbanck · 2021-11-12T14:45:20Z

src/clientHandler.ts

-        'Only one of rootModules and excludeRootModules can be set at the same time, please remove the conflicting config and reload',
-      );
-    }
+    const serverOptions: ServerOptions = this.getServerOptions();


Do you want to keep the type for now?

src/clientHandler.ts

github-actions · 2021-12-13T03:01:55Z

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.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

jpogran force-pushed the single_lang_client_redux branch from 7289733 to ce08d71 Compare November 10, 2021 22:12

jpogran self-assigned this Nov 10, 2021

jpogran added the enhancement New feature or request label Nov 10, 2021

jpogran changed the title ~~Single Language Client~~ Refactor to Single Language Client Nov 10, 2021

jpogran requested a review from a team November 10, 2021 22:18

jpogran marked this pull request as ready for review November 10, 2021 22:18

jpogran linked an issue Nov 10, 2021 that may be closed by this pull request

Drop support of (old) LS without multiple folders #816

Closed

jpogran mentioned this pull request Nov 10, 2021

Single LanguageClient #834

Closed

jpogran added this to In Progress in Terraform Editor Experience Public Roadmap Nov 10, 2021

dbanck requested changes Nov 11, 2021

View reviewed changes

jpogran force-pushed the single_lang_client_redux branch 2 times, most recently from f296eac to 914bbb5 Compare November 11, 2021 17:37

radeksimko approved these changes Nov 12, 2021

View reviewed changes

src/extension.ts Outdated Show resolved Hide resolved

jpogran and others added 15 commits November 12, 2021 09:46

update terraformextension interface

79e03f5

Apply suggestions from code review

eb6cbe3

Co-authored-by: Daniel Banck <dbanck@users.noreply.github.com>

Fold in feedback

b160ea8

Fold in feedback

95a966c

Fold in feedback

c799aea

Fold in feedback

89c5483

Apply suggestions from code review

8ccdd45

Co-authored-by: Daniel Banck <dbanck@users.noreply.github.com>

Fold in feedback

60e59d8

Fold in feedback

8258156

Fold in feedback

2d1dfbe

Fold in feedback

3357411

Fold in feedback

787ca86

Update src/extension.ts

fd08f01

Co-authored-by: Radek Simko <radek.simko@gmail.com>

update changelog

55b6155

jpogran force-pushed the single_lang_client_redux branch from f170a7a to 55b6155 Compare November 12, 2021 14:47

dbanck approved these changes Nov 12, 2021

View reviewed changes

jpogran added 3 commits November 12, 2021 11:39

Fold in feedback

7445e38

Fold in feedback

43f7947

Fold in feedback

03115f3

dbanck approved these changes Nov 12, 2021

View reviewed changes

jpogran merged commit ac5140e into main Nov 12, 2021

Terraform Editor Experience Public Roadmap automation moved this from In Progress to Done Nov 12, 2021

jpogran deleted the single_lang_client_redux branch November 12, 2021 17:41

github-actions bot locked as resolved and limited conversation to collaborators Dec 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor to Single Language Client #845

Refactor to Single Language Client #845

jpogran commented Nov 10, 2021 •

edited by radeksimko

Loading

dbanck left a comment

radeksimko left a comment •

edited

Loading

jpogran commented Nov 12, 2021

radeksimko commented Nov 12, 2021

dbanck left a comment

dbanck Nov 12, 2021

github-actions bot commented Dec 13, 2021

Refactor to Single Language Client #845

Refactor to Single Language Client #845

Conversation

jpogran commented Nov 10, 2021 • edited by radeksimko Loading

Refactor Language Client Workflow

Testing

dbanck left a comment

Choose a reason for hiding this comment

radeksimko left a comment • edited Loading

Choose a reason for hiding this comment

jpogran commented Nov 12, 2021

radeksimko commented Nov 12, 2021

dbanck left a comment

Choose a reason for hiding this comment

dbanck Nov 12, 2021

Choose a reason for hiding this comment

github-actions bot commented Dec 13, 2021

jpogran commented Nov 10, 2021 •

edited by radeksimko

Loading

radeksimko left a comment •

edited

Loading