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

Disable default-features for http-types #63

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joshtriplett
Copy link
Member

http-client doesn't use any of the default features.

@Fishrock123
Copy link
Member

That isn't true, we re-export some things (e.g. Body) which as additional functions based on features, unfortunately.

I suspect we could change this without anyone noticing but we should probably be careful.

@joshtriplett
Copy link
Member Author

joshtriplett commented Jan 28, 2021

Good catch, thank you.

It looks like the main issue there would be things like Body::from_file, which needs the fs feature. Making http-client depend on the fs feature of http-types shouldn't be a problem; that only causes http-types to depend on async-std, which will often be a dependency of http-client anyway. So, I'll change this PR to enable the fs feature, which should then avoid breakage.

(In theory, someone could rely on http_client::http_types::cookie or similar, but that seems much less likely compared to just using http_types directly.)

@joshtriplett joshtriplett force-pushed the http-types-no-default-features branch from 265076d to 8e2bd38 Compare January 28, 2021 08:38
@joshtriplett
Copy link
Member Author

@Fishrock123 Does this seem more reasonable? I think we can make this change without a semver-major bump.

@joshtriplett
Copy link
Member Author

joshtriplett commented Jan 28, 2021

Not sure what's causing the test failure. It looks like it happens on latest main, without this change; perhaps something changed elsewhere in the ecosystem that broke the http-client build.

EDIT: Ah, it's http-rs/tide#787 .

@Fishrock123
Copy link
Member

@joshtriplett Could you rebase?

http-client doesn't need the cookie feature. http-client also doesn't
*directly* use the fs feature, but it re-exports http_types::Body and
people might be relying on Body::from_file, so depend on the fs feature
for compatibility.
@joshtriplett joshtriplett force-pushed the http-types-no-default-features branch from 8e2bd38 to 250ce32 Compare February 17, 2021 06:37
@joshtriplett
Copy link
Member Author

joshtriplett commented Feb 17, 2021

@Fishrock123 Done.

@Fishrock123
Copy link
Member

@joshtriplett If we release this, is cargo smart enough to not upgrade someone to this version if they are pinning to a previous http-types for some reason?

@Fishrock123 Fishrock123 added this to the 7.0.0 milestone Feb 23, 2021
@joshtriplett
Copy link
Member Author

@Fishrock123 Cargo doesn't allow multiple compatible versions of the same crate in one dependency tree. If you're pinning an older http-types, you can't end up with a newer http-client that needs a newer http-types. (I don't know to what degree cargo's version resolution will decide to use an older http-client and to what degree it'll throw up its hands and say "nope, incompatible".)

@joshtriplett
Copy link
Member Author

Checking back on this: would it be possible to merge this to allow dropping cookie crates from projects that don't need them?

@Fishrock123
Copy link
Member

Fishrock123 commented May 18, 2021

Bumping a major here likely means bumping a major in surf, which I'd like to avoid until I have to for http-types 3.0 if possible.

@olanod
Copy link

olanod commented Jun 9, 2021

Is there a major milestone waiting to get to http-types 3.0? Similarly I'm trying to get rid of dependencies keeping things as light as possible and the latest http-types helps a bit changing the rand dependency that I wont be needing

@joshtriplett
Copy link
Member Author

@Fishrock123 As far as I can tell, now that this change no longer omits the fs feature, it shouldn't require bumping any major versions. Given that, would you consider this change for http-client?

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