Closed talex5 closed 4 months ago
Looks good to me!
I would even suggest making Cohttp_eio.Server.respond
's status
argument default to OK, just like Cohttp.Response.make
, so that one need only replace (Cohttp.Response.make (), body)
with Cohttp_eio.Server.respond ~body ()
to adapt to this change.
We might even make body
positional since it's required anyway, making the signature:
val respond :
?headers:Http.Header.t ->
?flush:bool ->
?status:Http.Status.t ->
_ Eio.Flow.source ->
response IO.t
So that the simplest form becomes Cohttp_eio.Server.respond body
.
That probably is a better API, but it's trying to implement the signatures in the shared cohttp package, so they would have to be extra functions with different names if we did that. Maybe something for a separate PR?
What is the status of this? I'd like to make a RC release of cohttp in the next couple of weeks. @mefyl and @talex5 do you think we should merge it as is or it needs further discussion?
AFAIR this looks fine to me and we can merge it.
@talex5 can you have a look at the conflict? I think after that we could merge
I rebased this. The conflict was due to #1025. If I understand the logic there correctly, this is what cohttp-lwt now does:
respond*
to create an HTTP 1.1 response, possibly missing the connection header, and returns it.The seems a bit odd. It would perhaps make more sense to create the response correctly in the first place, but that would require passing the request to respond
, etc, which would be an API change.
For cohttp-eio, I modified the (abstract) writer
type to include the request, so respond
can build a correct response by itself. The test is simplified a bit because (as with Lwt's responders) the response is always HTTP 1.1, so Http.Response.is_keep_alive
is always true
when the header is missing.
Thanks. I think it may be worth revising the api for cohttp-lwt but it could be done in a future 7.0 release
This fixes #998.
The first commit adds an abstract
response
type and then usestype response = Http.Response.t * Body.t
in cohttp-lwt and cohttp-eio to keep the final types the same (cohttp-async already defined this type anyway and so didn't need any updates).The second is just a few trivial changes to cohttp-eio to make the third commit diff smaller.
The third commit changes the
response
type in cohttp-eio totype response = writer -> unit
. This allows handlers to write the response inside the function, rather than returning a request to cohttp to write it later. That's useful because it allows e.g. streaming from an open file and then closing it afterwards.Partial application means that code using
respond_string
etc will continue to work as before.This also exposes a more polymorphic version of the
respond
function that accepts sub-types ofFlow.source
, so that callers don't need to cast the body./cc @mefyl