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

Support both http 0.2 and 1.0 #684

Merged
merged 1 commit into from
Nov 24, 2023
Merged

Support both http 0.2 and 1.0 #684

merged 1 commit into from
Nov 24, 2023

Conversation

Nemo157
Copy link
Contributor

@Nemo157 Nemo157 commented Nov 21, 2023

This implements the idea I had in #683, supporting both versions of http and with the planned breaking change to supporting TryFrom<http::request::Builder> instead of From<http::request::Builder> since it's non-breaking as a new feature.

@algesten
Copy link
Owner

Thanks!

The problem with this solve is that the implementations are mutually exclusive. That means if some project get diamond dependencies on ureq with different feature flags, the compilation might break completely.

cargo test --features http http-interop

The immediate problem can of course make one of the flags "win", but that wouldn't help impls that depend on one that doesn't win.

What are the drivers for solving the retaining of the old dependency?

@Nemo157
Copy link
Contributor Author

Nemo157 commented Nov 21, 2023

The problem with this solve is that the implementations are mutually exclusive.

They aren't, they're entirely independent. cargo test --all-features works fine and using both features together from one build will work.

What are the drivers for solving the retaining of the old dependency?

Avoiding breakage of code relying on the previous API. If it had been documented that the http-interop feature wasn't going to be semver compatible then I would have avoided using it in my crate and just performed the conversion manually. I assume my crate is not going to be the only one hit by this incompatibility, I'm likely just the first to notice as I have a CI job that tests against updated dependencies daily.

@Nemo157
Copy link
Contributor Author

Nemo157 commented Nov 21, 2023

Hmm, it's less than I thought, if I did this correctly these should be the only (published) crates that are likely currently broken with ureq 2.9.0:

> fd -0tf -Econfig.json '' crates.io-index | xargs -0 tail -qn1 | jq -r '.name as $name | .deps[] | select(.name == "ureq" and (.features | contains(["http-interop"]))) | $name'
crates-index
cargo-dl
cargo-features-manager
cargo-upgrades

EDIT: Quickly testing them, crates-index only has broken tests (and is the only library so there should be no transitive breakage), cargo-features-manager and cargo-upgrades both have broken normal builds.

@algesten
Copy link
Owner

Alright. Let's try to land this.

Would it be possible to duplicate src/http_interop.rs and have one file for http1 and one for http02, and that would give us a smaller diff?

@Nemo157
Copy link
Contributor Author

Nemo157 commented Nov 22, 2023

Done. It doesn't really reduce the diff but it does probably make it more readable.

@algesten
Copy link
Owner

I'll do the rest here. Thanks!

@algesten algesten merged commit 87108e0 into algesten:main Nov 24, 2023
30 of 47 checks passed
@algesten
Copy link
Owner

In released version 2.9.1, renamed the http feature to http-crate since in the context of ureq, the name "http" is a bit too generic.

@Nemo157 Nemo157 deleted the multi-http branch November 24, 2023 11:43
@MarijnS95
Copy link
Contributor

Did you forget to remove this planned removal from https://github.com/algesten/ureq/blob/main/FUTURE.md, after borrowing the suggested implementation from it?

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

Successfully merging this pull request may close these issues.

3 participants