Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Usability nits #67

Open
tgeoghegan opened this issue May 23, 2024 · 3 comments
Open

Usability nits #67

tgeoghegan opened this issue May 23, 2024 · 3 comments

Comments

@tgeoghegan
Copy link
Contributor

I want to note a grab bag of minor usability nits to see whether the maintainer(s) care about them and/or agree with my suggestions. These would involve changing or adding to public API so there are meaningful maintainability considerations here.

  • making clients import both bhttp and ohttp is a chore. Is anyone ever going to use ohttp without bhttp? In particular, this is annoying because I wind up having to define separate enum variants in my error type for ohttp::Error and bhttp::Error, as does glean/Viaduct.
  • bhttp has its own bhttp::StatusCode which is an alias for u16. What if this was http::StatusCode instead? Or perhaps there's a good reason that bhttp/ohttp avoid depending on crate http.
  • Continuing from that, it'd be nice to have adapters from things like bhttp::Message to things like http::Response or http::Request. These would be easy to gate behind a feature if there's a concern about dragging http into everyone's Cargo.lock.
  • decapsulating responses requires unnecessary ceremony in the common case where the response is small and I have the whole thing in memory. See glean/Viaduct: you already have the whole encapsulated response in memory, so it'd be nice to have a one-shot call on ClientResponse that does the Cursor dance for you and spits out a [Message] (or, in the context of Handling ControlData::Request when decapsulating responses is awkward #66, a more specific response type).
@martinthomson
Copy link
Owner

In general, I'm trying to avoid pulling in a whole lot of dependencies, which the http-related changes would. An adapter (or From/Into implementations) would be OK with a feature gate.

As for ohttp vs. bhttp, the two are completely independent from a code perspective. The convenience functions you are talking about adding might end that, so we can maybe wrap one error in the other for those functions.

I don't have a ton of spare time for doing this work, but I'd be happy to help out with reviews and whatnot.

@DanGould
Copy link
Contributor

What if this was http::StatusCode instead? Or perhaps there's a good reason that bhttp/ohttp avoid depending on crate http.

While avoiding a dependency on http is noble and admirable, I can't think of any environment where bhttp/ohttp would be used without a dependency on http somewhere in the tree.

@martinthomson
Copy link
Owner

Our use of this crate in Firefox is an example of exactly that, as it happens. Pulling in http would mean a much bigger binary. I'm OK with these conveniences being present, but they would need to be optional, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants