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

Implement Service for Client #226

Closed
ebkalderon opened this issue Sep 24, 2020 · 0 comments · Fixed by #313
Closed

Implement Service for Client #226

ebkalderon opened this issue Sep 24, 2020 · 0 comments · Fixed by #313
Assignees
Labels
enhancement New feature or request

Comments

@ebkalderon
Copy link
Owner

It seems like a design oversight that the Client type does not implement the tower_service::Service trait. Implementing this trait would enable compatibility with the tower middleware, and hopefully simplify the request dispatching logic.

Unlike LspService as described in issue #177, Client should be perfectly capable of having a unary request/response model. In this case, the Request type parameter would be ClientRequest and the associated types would be as follows:

type Response = Option<Response>; // Response is `None` if the request was a notification.
type Error = crate::jsonrpc::Error;
type Future = Pin<Box<dyn Future<Output = Result<Self::Response, Self::Error>> + Send>>;

It would be great if it were possible to implement Service generically, specializing over R: lsp_types::Request and N: lsp_types::Notification and accepting Params from both as input, but this is impossible to express in the Rust type system, as far as I can tell. I think the above approach is good enough solution for our use case.

We should think about whether ClientRequest should be redesigned to not include the id field for requests, in favor of having the Client manage it internally instead, using the request_id field. This prevents callers from submitting the same request ID twice, preventing accidental collisions and improving type safety.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant