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

feat: wrap_connector from builder WantsProtocols1 #260

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

Conversation

Fishrock123
Copy link
Contributor

There is a use case for wrapping libraries where they may not bring in either http or http2 features, but still want to set advanced configuration but with their own lower-level connector.

This was discovered when trying to expose builder::with_server_name() in reqwest, which seems impossible right now without enabling extra features.

There is a use case for wrapping libraries where they may not bring in either `http` or `http2` features, but still want to set advanced configuration but with their own lower-level connector.
@djc
Copy link
Member

djc commented Mar 18, 2024

This is part of a trio of smallish PRs:

Just to make sure we're not losing the forest for the trees, what exactly is the problem that you're trying to solve here? Adding a bunch of API surface like this has me wary that this might not be the best way to address the underlying needs.

@Fishrock123
Copy link
Contributor Author

These PRs are separate because they are in effect all completely unrelated, and could just as well be discovered independently.

This one is the most important. Reqwest may not be able to use the builder API without this change. (I am not a request author, so I can't speak to if they would accept the PR with added, unecessary, hyper-rustls features.)

Put more broadly, if a wrapping client wants to implement the HttpConnector itself and not pull in unnecessary code, this is required, because otherwise you cannot advance the builder's static state far enough.

@cpu
Copy link
Member

cpu commented Mar 26, 2024

This one is the most important. Reqwest may not be able to use the builder API without this change. (I am not a request author, so I can't speak to if they would accept the PR with added, unecessary, hyper-rustls features.)

If this change is motivated by a need in Reqwest, could you open a draft PR sketching out that side of the equation that takes a git dep on this branch? I think if my understanding is correct we would want to get stronger signal from the consuming projects before adding more API surface here.

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