Skip to content

Consider replacing http-types crate with http crate #1644

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

Closed
svix-jplatte opened this issue Apr 19, 2024 · 14 comments · Fixed by #2331
Closed

Consider replacing http-types crate with http crate #1644

svix-jplatte opened this issue Apr 19, 2024 · 14 comments · Fixed by #2331
Labels
Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback.

Comments

@svix-jplatte
Copy link

svix-jplatte commented Apr 19, 2024

Hey, I noticed you are using the unmaintained http-types crate for a little bit of codegen, plus its StatusCode and Method types. This is a tiny fraction of the functionality it includes, especially if you consider all of its dependencies.

There is a much more minimal crate for HTTP types that is actively maintained and seems like it should fulfill all of your needs: https://crates.io/crates/http. Would you consider switching to it? It would of course be a breaking change. I might be able to post a PR if you're interested.

@svix-jplatte
Copy link
Author

For extra context, the http crate is not just an alternative crate, it's maintained by Sean McArthur alongside hyper, the most popular HTTP implementation for Rust by far. It's also used in reqwest, axum, actix-web, the AWS SDK, wiremock and many more crates / projects.

@heaths Hope you don't mind the ping, I think this is somewhat important as the current state of affair raises clear red flags for anybody who reviews their dependency trees.

@heaths
Copy link
Member

heaths commented May 13, 2024

I'd consider a PR, but there'll be a lot of changes coming up. We're planning to reduce dependencies as much as possible and practical and, IMO, pulling a crate just for a few simple types isn't worth the extra risk and build time. All that work is happening in a separate feature branch which we'll talk about in the not-so-distant future more. So whether you want to spend the time on a PR or not is up to you. As long as it isn't huge I'm happy to review, but it's not a high priority for me.

That said, this code - these crates - will be community-maintained (including me, since this was always unofficial) for the foreseeable future so it may be good to remove a dead dependency as you suggest.

@cataggar
Copy link
Member

Two years ago, I took us the other way. We were removing the http dependency in favor of our own types in azure_core and some types from http_types. Probably worth revisiting some of the notes:

cc @yoshuawuyts

@heaths
Copy link
Member

heaths commented May 15, 2024

I appreciate that Rust, like JS, is a smattering of smaller crates comprising libraries; however, while we've always preferred first-party or even no dependencies, the "recent" events - not just xz - certainly up the priority. Governance is important, as is maintenance. I see http-types hasn't been touched in a couple years (though it's fairly basic, relatively speaking, and maybe hasn't needed it). Is that expected to be maintained (even review/merge PRs as needed) going forward? I see @yoshuawuyts is the biggest contributor.

I've looked through the other issue and PR you mentioned, but I don't understand in what scenarios compatibility with http-types is necessary (based on this comment). @cataggar care to elaborate?

@heaths heaths added Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback. labels Jun 28, 2024
@svix-jplatte
Copy link
Author

Gentle ping on this

@heaths
Copy link
Member

heaths commented Sep 20, 2024

This is not currently a priority. The issue is left open as a tracking issue because I do want to get to this eventually, but we're trying to get our first betas out built on the new TypeSpec code emitter like all our other languages use (for their respective languages).

I'd rather remove the dependency on http or http-types entirely. It's not worth the build time for 2 simple types. What we'll end up with will likely be compatible with what we have now, so it'd be easy enough to map from one representation to another.

From the OP,

It would of course be a breaking change. I might be able to post a PR if you're interested.

All code in main is unofficial and unsupported. Code we'll be releasing from feature/track2 will be official but beta, so there should be no expectations to avoid breaking changes.

@heaths
Copy link
Member

heaths commented Dec 2, 2024

To summarize my read down the rabbit hole of comments e.g., #849 (comment), the gist is that we prefer http-types's enums over http's constants. That certainly improves the DX in a lot of ways.

@pront
Copy link

pront commented Mar 3, 2025

👋 Would you consider removing the dependency on http-crates completely? Last update was over 3y ago https://crates.io/crates/http-types/versions. This forces projects to keep old dependencies in their trees e.g. rand 0.7.3.

heaths added a commit to heaths/azure-sdk-for-rust that referenced this issue Mar 13, 2025
Resolves Azure#1644 and replaces Azure#2233. The latter PR was a good attempt but we decided internally we don't want to take a dependency on a different crate - `http` ; though, we do have a transitive dependency on it already - for something so simple.

This starts as a copy from http_types with appropriate attribution. We may want to remove a bunch of Methods we don't need, though, at some point.
heaths added a commit to heaths/azure-sdk-for-rust that referenced this issue Mar 13, 2025
Resolves Azure#1644 and replaces Azure#2233. The latter PR was a good attempt but we decided internally we don't want to take a dependency on a different crate - `http` ; though, we do have a transitive dependency on it already - for something so simple.

This starts as a copy from http_types with appropriate attribution. We may want to remove a bunch of Methods we don't need, though, at some point.
@heaths heaths closed this as completed in 811cb15 Mar 14, 2025
@pront
Copy link

pront commented Mar 31, 2025

Thank you for working on this! Do you plan to release a version that includes this refactoring anytime soon?

@heaths
Copy link
Member

heaths commented Mar 31, 2025

Our normal release cycle for Azure SDKs is the first full week of the month. We should be releasing a new beta then.

@pront
Copy link

pront commented Apr 16, 2025

One follow up here, do plan to release a new version for:

(since they still depend on the azure_core 0.21.0)

@heaths
Copy link
Member

heaths commented Apr 16, 2025

Those are not the correct crates. The Storage team decided to rename it to azure_storage_blob consistent with most other Azure SDK languages and the product name itself (singular).

/cc @vincenttran-msft

@vincenttran-msft
Copy link
Member

@pront Hi Pavlos, here is the current, in-development Azure Storage Blob client library crate: https://crates.io/crates/azure_storage_blob

Please note that not all features may be implemented or work as intended currently. Thanks!

@pront
Copy link

pront commented Apr 16, 2025

@pront Hi Pavlos, here is the current, in-development Azure Storage Blob client library crate: https://crates.io/crates/azure_storage_blob

Please note that not all features may be implemented or work as intended currently. Thanks!

Interesting, this is what we depend on today:
https://github.com/vectordotdev/vector/blob/master/Cargo.toml#L258-L262

Do we have to replace the storage ones with https://crates.io/crates/azure_storage_blob?

Let me know if anything else stands out.
Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Client This issue points to a problem in the data-plane of the library. design-discussion An area of design currently under discussion and open to team and community feedback.
Projects
Status: Done
5 participants