Support h2.0.13.0 #381
Support h2.0.13.0 #381
Conversation
| @@ -85,5 +85,5 @@ let forward_body_h2 | |||
| response | |||
| (H2.Body.Writer.write_string body) | |||
| (H2.Body.Writer.write_bigstring body) | |||
| (H2.Body.Writer.flush body) | |||
| (fun f -> H2.Body.Writer.flush body (fun _ -> f ())) | |||
There was a problem hiding this comment.
Doesn't this change make it incompatible with h2<0.14.0 ?
There was a problem hiding this comment.
flush takes unit -> unit before 0.13 and [Something | Something] -> unit (i don't recall exactly) with 0.13, but fun _ -> f () is of type 'a -> unit so it typechecks with both version (tried locally).
There was a problem hiding this comment.
OK, you might also need to add a lower bound, see https://github.com/ocaml/opam-repository/pull/27202/files
There was a problem hiding this comment.
Could indeed reproduce that 0.8.0 doesn’t work. Pushed a new version of the commit.
|
Funny, it has fixed the CI. |
| @@ -17,7 +17,7 @@ depends: [ | |||
| "dune" {>= "2.7.0"} # --instrument-with. | |||
| "gluten" | |||
| "gluten-lwt-unix" | |||
| "h2" {< "0.13.0"} | |||
| "h2" {>= "0.9.0" & < "0.14.0"} | |||
There was a problem hiding this comment.
Maybe remove the upper bound as we so far don't have a reason for that? WDYT?
There was a problem hiding this comment.
As the version is still <1 there can be breaking changes in any future release. Something to keep in mind.
There was a problem hiding this comment.
Sure! In the past I've set upper version constraints on as-yet unreleased versions of dependencies that were <1. To me that helped me not get distracted by new breaking releases. However, it also ensured I didn't test against new releases until much much later when I remembered why I put the upper bound.
In the end I think it's usually easier to put an upper bound when needed than remembering to test new releases when there's a speculative upper bound. YMMV.
There was a problem hiding this comment.
I think this MR is a good example on why it’s a good idea to have a strict upper bound: 0.13 does introduce a breaking change. 😅
|
Out of curiosity, what’s the next step? Having a beta9 with this change could unlock us using the latest release of Dream, so I’d be happy to help to make this happens (if this aligns with the project roadmap, obviously). |
|
We'll wait for aantron to check in. Hopefully he is able to soon. |
|
Note that a similar change could be made on line 77 to ensure compatibility with |
The hard constraint on
h2prevents us to update Dream to its latest beta. I’d like to propose this minimal patch to adapt Dream-httpaf to the breaking change of h2 0.13.There might be better way to do it, but I don't know these codebases, hence this first take.
Let me know what you think!