cohttp-eio: add Client.make_generic and HTTPS support #1002
Conversation
This is to allow other resolution systems in future.
The new `Client.make_generic` allows the user to provide their own function for resolving URIs to flows. The convenience wrapper `make` now taken an `https` argument, which is a function that can be used to wrap raw sockets with a TLS library. The `client_tls.ml` example shows how to use this with tls-eio.
| @@ -1,9 +1,29 @@ | |||
| open Eio.Std | |||
There was a problem hiding this comment.
Can we avoid opening here, and leave Eio.Switch below?
There was a problem hiding this comment.
It would be a bit ugly - several other places would get a fair bit longer too.
|
Thanks! Just in time, I was getting ready to release the new beta :) |
| uses [net] to make connections. | ||
| https: | ||
| (Uri.t -> [ `Generic ] Eio.Net.stream_socket_ty r -> _ Eio.Flow.two_way) | ||
| option -> |
There was a problem hiding this comment.
| option -> | |
| -> |
Since it is no longer optional, why not dropping the option from the signature?
There was a problem hiding this comment.
https support is optional (you don't have to use it); see #984 (comment):
I don’t have too strong of an opinion, I think the optional is cleaner but perhaps https is so pervasive nowadays that it is good to keep it explicit instead.
There was a problem hiding this comment.
if the plan is to have a separate cohttp-eio-tls package I suggest making the parameter optional - otherwise it's a bit clumsy for HTTP calls to have to thread that ~https:None parameter everwhere.
There was a problem hiding this comment.
Yes, I agree. I was recalling what we had discussed incorrectly
There was a problem hiding this comment.
Is this signature ok as is, or do we need to change https into ?https?
There was a problem hiding this comment.
I think I would prefer ?https but I also understand the rationale to keep it explicit as the error mode (the call will just hang) is not great right now.
There was a problem hiding this comment.
Let's leave it as is. We can make it optional in due course once there is the tls package if we can think of a better way to deal with it
| in | ||
| Eio.Switch.run @@ fun sw -> | ||
| let resp, body = | ||
| Client.get ~sw client (Uri.of_string "https://example.com") |
There was a problem hiding this comment.
I am a bit sad with the amount of boilerplate we have to come up to do a simple https call. Could we somehow ship a default authenticator as well? The current API doesn't look very simple to use.
There was a problem hiding this comment.
We can't ship a default authenticator without adding extra dependencies to cohttp-eio. The plan is to have another package for that later (either cohttp-eio-tls, or maybe something conduit-like); this PR is just making that possible.
There was a problem hiding this comment.
I think we could merge this and release it as beta, so that people can test it out in the meantime and inform possible api refinements. But I agree with @samoht, and I think it would be nice to have cohttp-eio-tls before we release the new stable 6.0.0
There was a problem hiding this comment.
That sounds like a good plan. I'm already using that PR in a project of mine and it's working great 👍 Thanks for this!
CHANGES: - cohttp-eio: Complete rewrite to follow common interfaces and behaviors. (mefyl mirage/ocaml-cohttp#984) - cohttp-eio: Add Client.make_generic and HTTPS support. (talex5 mirage/ocaml-cohttp#1002)
CHANGES: - cohttp-eio: Complete rewrite to follow common interfaces and behaviors. (mefyl mirage/ocaml-cohttp#984) - cohttp-eio: Add Client.make_generic and HTTPS support. (talex5 mirage/ocaml-cohttp#1002)
#984 (comment) said:
However, the TLS support wasn't included in the end, so I'm opening a separate PR for it here.
/cc @SGrondin