Organize Static Features #1073
Organize Static Features #1073
Conversation
This moves all custom Static Feature classes to a central folder. It also consolidates the Semantic Token manifest parsing helper methods and moves them to the CustomSemanticTokens class.
| @@ -37,6 +37,7 @@ export class ClientHandler { | |||
| private lsPath: ServerPath, | |||
| private outputChannel: vscode.OutputChannel, | |||
| private reporter: TelemetryReporter, | |||
| private manifest: PartialManifest, | |||
There was a problem hiding this comment.
Since this is now used outside the scope of semantic tokens I wonder if it's worth calling it e.g. ManifestWithSemanticTokens? or making a copy of that interface here which can be expanded and used elsewhere later - presumably one that is compatible with PartialManifest within semanticTokens.
I suppose there are other places where we access the manifest for some other properties.
There was a problem hiding this comment.
I would agree if we didn't have more work to break up the dependencies inside ClientHandler.
ClientHandler only needs to know about this because of the StaticHandler registrations. Building up a StaticHandler should be done outside ClientHandler and passed in rather than passing in all information required to build several StaticHandlers.
I could do this here now, or do that work with the next planned PR for refactoring ClientHandler. I prefer to make those changes in the next PR.
This extracts the LanguageClient creation from ClientHandler and puts it directly inside the activation point. In order to implement hashicorp/terraform-ls#722 we need to add a new StaticFeature to register a client-side command for terraform-ls to call when it knows the `terraform.providers` view needs to be refreshed. StaticFeatures are registered using the LanguageClient.registerFeature method, which means the ClientHandler class needs to create the StaticFeature. The StaticFeature needs both the LanguageClient and `terraform.providers` view created before it can be initialized. The LanguageClient is created by the ClientHandler class. This all results in a cyclic dependency. We could resolve this cyclic dependency by adding more responsibility to ClientHandler, but this introduces more complexity to the class. This will become increasingly hard to deal with as more aspects like StaticFeature are added. We resolve this by extracting the creation of LanguageClient and moving dependent features like the StaticFeatures to the main activation method. This puts all the parts that rely on each other in the same place where the data needed is located to make decisions about whether they are activated or not. This builds on work done in: - #1073 - #1074 - #1075 - #1079
|
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. |
This moves all custom Static Feature classes to a central folder. It also consolidates the Semantic Token manifest parsing helper methods and moves them to the CustomSemanticTokens class.