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

Avoiding conflicting features; allow multiple HTTP clients at once #221

Closed
marioortizmanero opened this issue Jul 7, 2021 · 6 comments
Closed
Assignees
Labels
enhancement New feature or request Stale
Milestone

Comments

@marioortizmanero
Copy link
Collaborator

marioortizmanero commented Jul 7, 2021

Something that has been troubling me for a while is that we have conflicting features in rspotify, but features in Rust must be additive:

  • Currently, the user can't enable multiple clients at the same time. If the features client-ureq and client-reqwest are enabled at the same time, they'll get hit by a compiler error warning them not to do so.
  • It should work like reqwest does for its blocking feature. It's not a toggle. Instead, it enables the blocking module so that you may use it in combination of the async one.

This is important because the Rust compiler may merge features of a crate when it's duplicated in the dependency tree. If your crate has a dependency somewhere on rspotify with reqwest and at the same time somewhere else rspotify with ureq, that will never possibly work because you can't merge client-ureq with client-reqwest. This is explained a few times in this post, for example here.

I thought this was more complicated than it actually is (still not easy). We actually have a base HTTP trait already, so what if we make the Spotify client generic over it? Here's a playground link that explains it: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=de0e9c0a596a5004b3e4af41071415d6. It might simplify a lot of things, really! The only downside is that the syntax to initialize the client will be a bit ugiler to specify the HTTP client.

Related issues:

@marioortizmanero marioortizmanero added the enhancement New feature or request label Jul 7, 2021
@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Jul 7, 2021

By the way, this is completely unrelated to #215. You can still merge these and I'll work on this afterwards. There's no rush to close this issue as long as it's done before the new version; I'll do so when I have a bit of time.

@marioortizmanero
Copy link
Collaborator Author

I've made this repo to prove how this can be a problem, even with the new resolver: https://github.com/marioortizmanero/resolver-v2-conflict

@bjorn3
Copy link

bjorn3 commented Oct 15, 2021

Would it be possible to publish two crates based on the same source code? One with the sync feature forcefully enabled and one with the async feature forcefully enabled? This is basically what rapier does to handle 2d/3d and f32/f64 with the same source. For example the 2d variant with f32 floats uses this Cargo.toml: https://github.com/dimforge/rapier/blob/b59e813fd336419fcd44a78adc0b2c88b1d1cdca/build/rapier2d/Cargo.toml#L40-L43 while the 3d variant with f32 floats uses: https://github.com/dimforge/rapier/blob/b59e813fd336419fcd44a78adc0b2c88b1d1cdca/build/rapier3d/Cargo.toml#L40-L43

@marioortizmanero
Copy link
Collaborator Author

marioortizmanero commented Oct 15, 2021

I tried to do that in a million ways, including how rapier does it in your links, but I ultimately gave up. I couldn't find any examples either, which made it harder. It's pretty cool that rapier managed to do it. I might have been doing it the wrong way because I didn't even know about required-features.

Anyhow, actually fixing the problem instead of using a hack like that will be better in the long run. No need for feature combinations when testing (just --all-features), nor documenting how it works and similars.

@marioortizmanero
Copy link
Collaborator Author

This is super interesting and might actually fix our issue! https://blog.rust-lang.org/inside-rust/2022/07/27/keyword-generics.html

@github-actions
Copy link

github-actions bot commented Jul 4, 2023

Message to comment on stale issues. If none provided, will not mark issues stale

@github-actions github-actions bot added the Stale label Jul 4, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants