Upstream Jane Street Changes #227
Conversation
lib/dune
Outdated
| @@ -1,6 +1,6 @@ | |||
| (library | |||
| (name httpaf) | |||
| (public_name httpaf) | |||
| (libraries | |||
| angstrom faraday bigstringaf) | |||
| (libraries angstrom faraday bigstringaf base sexplib) | |||
There was a problem hiding this comment.
Yes, for the [@deriving sexp] on the types. However, I don't think we need the base dependency, so I've removed it.
There was a problem hiding this comment.
Hmmhmm, but do we really need [@deriving sexp]?
It is of very limited interest. As far as I can see, there is no internal use for the deriver in the project - so it responds to an external demand (I suppose, in this case, JaneStreet). However, there is no demand (in the issues) for such a deriver and I'm not personally in favour of using it.
There was a problem hiding this comment.
As far as I can see, there is no internal use for the deriver in the project - so it responds to an external demand
They're used in some expect_tests that I'm still in the process of upstreaming.
so it responds to an external demand (I suppose, in this case, JaneStreet).
All of the people who have contributed to httpaf currently work at Jane Street, so the internal / external divide isn't totally clear. I also won't pretend to speak on their behalf, I'm just doing the work of pushing the changes that they already made.
Hmmhmm, but do we really need
[@deriving sexp]?
The dependency is very small, and if you have code that uses the sexp deriver, then having the serializers and deserializers directly in the library means that you don't have to re-declare the types to add the derivers and keep them in sync.
There was a problem hiding this comment.
All of the people who have contributed to httpaf currently work at Jane Street
That's a little simplistic...
I also won't pretend to speak on their behalf, I'm just doing the work of pushing the changes that they already made.
There are several questions here:
- how do maintainers/authors view
httpafand its community. - JaneStreet surely has an internal use of
httpafbut it seems that not all of it has been exported here (furthermore, Support upgrades via I/O operations #159 or Http upgrades #203 exist and do not add the deriver) httpafis used by other projects (notably dream) for which adding a dependency (even a small one) has real implications.
For example, as far as Mirage is concerned, we've already had problems with sexplib0 (which we've of course fixed).
The dependency is very small, and if you have code that uses the sexp deriver, then having the serializers and deserializers directly in the library means that you don't have to re-declare the types to add the derivers and keep them in sync.
This remains a very specific use. Once again, no one in the community has asked for such a deriver, even if it's only in the issues. It should also be understood that it can be frustrating to have a dependency imposed on you as a long-standing user (and contributor in a way - as far as faraday, angstrom or bigstringaf are concerned) of httpaf for a reason that is as obscure as it is internal to JaneStreet.
|
@seliopou Can we see this getting merged? |
|
On the topic of upstream, I feel that some of the great work done by @anmonteiro needs to be upstreamed as well. In addition, it would be nice if we could add someone to this organisation to develop more sustainably. |
|
FWIW, I'm reluctant to add sexplib and derivers into the library. If you really need them, I'd appreciate an approach as done by ipaddr -- put the derivers into a separate opam package (this indeed means that you have to expose the types and re-alias them). E.g. from ocaml-tls all the sexplib stuff has been removed (mirleft/ocaml-tls#473), leading to a reduction of 30% of the binary size -- mirage/mirage-tcpip#505 also removing ppx lead to a drastic reduction of compilation time and binary size. |
|
I'm pretty eager as well to see http upgrades introduced in some way to this library. Looking at the diff, it looks like it'd be pretty easy to separate out the bit introducing sexplib from the bit that introduces upgrades. Why don't we separate the the PR into two and so that at least the feature that people aren't having concerns about (the http upgrades) can find its way in. To move this along more quickly, @TyOverby, do you mind if I create a patch on top of your commits to intro a version of this PR without the sexplib dependency? |
|
@yiblet: sounds good, go right ahead! |
Jane Streets internal copy of httpaf has accumulated some changes that we'd like to upstream to prevent further divergence. These changes mostly involve support for upgrading connections to support websockets, and adding sexp derivers for many types.
This PR is mainly here to collect all the changes, and we might want to split them up into smaller, more targeted PRS.