Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow subscribing to updates of module.calls data #722

Closed
radeksimko opened this issue Nov 22, 2021 · 2 comments · Fixed by #909
Closed

Allow subscribing to updates of module.calls data #722

radeksimko opened this issue Nov 22, 2021 · 2 comments · Fixed by #909
Assignees
Labels
enhancement New feature or request good first issue Good for newcomers modules Functionality related to the module block and modules generally workspace/executeCommand
Milestone

Comments

@radeksimko
Copy link
Member

radeksimko commented Nov 22, 2021

Depends on #723


Use-cases

#632 introduced a custom command to allow clients obtain data about module calls. Clients may call this command repeatedly at any time and it's relatively cheap to do so. However the client may not necessarily know when is the right time to do so.

Clients should be able to "subscribe" to receive data updates whenever this data changes.

Attempted Solutions

Clients ask repeatedly - can lead to either excessive amount of requests and/or outdated info.

Proposal

The server already has mechanisms allowing it to detect state changes which affect the data returned from module.calls via hooks

type ModuleChangeHook func(oldMod, newMod *Module)
type ModuleChangeHooks []ModuleChangeHook
func (mh ModuleChangeHooks) notifyModuleChange(oldMod, newMod *Module) {
for _, h := range mh {
h(oldMod, newMod)
}
}

We can introduce additional optional argument update-command-id alongside uri

func ModuleCallsHandler(ctx context.Context, args cmd.CommandArgs) (interface{}, error) {

which clients can use to register a particular client-side command ID which the server will call and pass updated data that way.

Alternatively we could follow the LSP spec a bit more closely and use that command just to trigger client-side re-reading of data, i.e. have client call module.calls again. This does require another RPC roundtrip, but aligns more with e.g. semantic tokens or code lens refresh.

Implementation Notes

We can try to calculate the response before sending and compare hash that client could also calculate, to avoid sending the duplicate data repeatedly.

@radeksimko radeksimko added the good first issue Good for newcomers label Dec 1, 2021
@radeksimko radeksimko added the modules Functionality related to the module block and modules generally label Mar 2, 2022
@jpogran jpogran self-assigned this Apr 19, 2022
jpogran added a commit to hashicorp/vscode-terraform that referenced this issue Apr 28, 2022
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
@jpogran
Copy link
Contributor

jpogran commented Apr 29, 2022

While we can hook this up quickly because of 722, until #725 is done we won't detect new information to show anyway.

@jpogran jpogran assigned jpogran and unassigned jpogran Apr 29, 2022
@jpogran jpogran added this to the v0.28.0 milestone May 4, 2022
@jpogran jpogran linked a pull request May 4, 2022 that will close this issue
@github-actions
Copy link

github-actions bot commented Jun 9, 2022

I'm going to lock this issue 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 similar to this, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request good first issue Good for newcomers modules Functionality related to the module block and modules generally workspace/executeCommand
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants