Skip to content

Remove the version handshake from the RPC protocol#1775

Closed
gpetiot wants to merge 3 commits intoocaml-ppx:mainfrom
gpetiot:rpc-no-version
Closed

Remove the version handshake from the RPC protocol#1775
gpetiot wants to merge 3 commits intoocaml-ppx:mainfrom
gpetiot:rpc-no-version

Conversation

@gpetiot
Copy link
Copy Markdown
Collaborator

@gpetiot gpetiot commented Aug 23, 2021

This simplifies the RPC protocol by removing the whole "version handshake" part, the old `Version is not a command anymore but if some software used the csexp representation of the version command it will just get ignored.

This is necessary for the usecase that mdx has for ocamlformat-rpc (realworldocaml/mdx#335)

@gpetiot gpetiot mentioned this pull request Aug 23, 2021
Closed
@gpetiot gpetiot requested review from Julow and jberdine October 29, 2021 15:03
@gpetiot gpetiot marked this pull request as ready for review October 29, 2021 15:03
Julow
Julow reviewed Nov 3, 2021
View reviewed changes
Copy link
Copy Markdown
Collaborator

@Julow Julow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm in favor but I don't see what it's helping in the linked Mdx PR ?

However, I think the rpc API must be versioned (even if we support one version at a time), mainly to allow clients to support several versions of the protocol.
Is it reasonable to send from the server a message containing the version of the protocol as soon as the client connects ? Otherwise, we'll still need the version command for that.

@Julow
Copy link
Copy Markdown
Collaborator

Julow commented Nov 3, 2021

Is it reasonable to send from the server a message containing the version of the protocol as soon as the client connects

Actually, I think we should keep the version command as it is. The RPC is released at the same time as ocamlformat, clients that want to support several versions of ocamlformat will have to support several versions of the RPC too.

I agree that the server doesn't need to support several versions of the client. If it really needs to be simplified, what about:

  • Move the command to the main protocol (remove the "wait for version" phase).
    Maybe some clients don't need to bother with that because they use Opam constraints or vendor ocamlformat ? Or they cached the result.
  • Document that the command doesn't take an argument anymore: it has a constant response.

@gpetiot
Copy link
Copy Markdown
Collaborator Author

gpetiot commented Nov 3, 2021

I don't remember all the details but I think what happened for our mdx usecase realworldocaml/mdx#335 is that (for some reason I forgot) we need an intermediate layer between the ocamlformat-rpc server and the client, and we can't have the handshake made in these both layers, and it doesn't work either to have it in only one, and the modules used would not be the same.

@gpetiot gpetiot marked this pull request as draft December 16, 2021 10:23
@gpetiot gpetiot closed this Dec 17, 2021
@gpetiot gpetiot deleted the rpc-no-version branch December 17, 2021 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Julow Julow Julow left review comments

@jberdine jberdine Awaiting requested review from jberdine

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@gpetiot @Julow @facebook-github-bot