Add option to select HTTP client type#1079
Conversation
Currently `reqwest::Client` is the only type we allow as the underlying HTTP client. Some consumers of Progenitor may want to use `reqwest_middleware` to enable conveniences like automatic retries and support for `tracing`. Add a new `with_client_type` argument for `GenerationSettings` to allow callers to select which of these types to use as the client.
Reqwest added the `from_parts` and `build_split` methods on `RequestBuilder` for wasm targets with v0.12.5. Bump Reqwest on the wasm example to that version.
ahl
left a comment
ahl
left a comment
There was a problem hiding this comment.
I think there are significant obstacles to consider. For example, I don't think reqwest-middleware should become a dependency in omicron.
I would suggest we consider other approaches to allow for more inspection of CLI requests.
| format!("bytes = \"{}\"", DEPENDENCIES.bytes), | ||
| format!("futures-core = \"{}\"", DEPENDENCIES.futures), | ||
| format!("reqwest = {{ version = \"{}\", default-features=false, features = [\"json\", \"stream\"] }}", DEPENDENCIES.reqwest), | ||
| format!("reqwest-middleware = {{ version = \"{}\", default-features=false, features = [\"json\"] }}", DEPENDENCIES.reqwest_middleware), |
There was a problem hiding this comment.
I don't think we want to always include this dependency.
| futures-core = { workspace = true } | ||
| percent-encoding = { workspace = true } | ||
| reqwest = { workspace = true } | ||
| reqwest-middleware = { workspace = true } |
There was a problem hiding this comment.
I don't think this should be a mandatory dependency
| description = "An OpenAPI client generator - client support" | ||
|
|
||
| [dependencies] | ||
| anyhow = { workspace = true } |
There was a problem hiding this comment.
This feels like a step in the wrong direction. If we need a generic error, could we use Box?
| impl Default for ClientType { | ||
| fn default() -> Self { | ||
| Self::Reqwest | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
I thought you could derive default on enums and use a marker annotation
| ClientType::Reqwest => quote! { reqwest::Client }, | ||
| ClientType::ReqwestMiddleware => quote! { reqwest_middleware::ClientWithMiddleware }, |
There was a problem hiding this comment.
| ClientType::Reqwest => quote! { reqwest::Client }, | |
| ClientType::ReqwestMiddleware => quote! { reqwest_middleware::ClientWithMiddleware }, | |
| ClientType::Reqwest => quote! { ::reqwest::Client }, | |
| ClientType::ReqwestMiddleware => quote! { ::reqwest_middleware::ClientWithMiddleware }, |
| hyper = { workspace = true } | ||
| progenitor-client = { workspace = true } | ||
| reqwest = { workspace = true } | ||
| reqwest-middleware = { workspace = true } |
There was a problem hiding this comment.
what fails if this is not present?
|
I think I can get this working with pre and post hooks, closing this out. |
|
Supporting ReqwestClientWithMiddleware would make sense if you'd ask me. I'm trying to implement a client for an API that requires OAuth2 token requests and renewals. Currently having a hard time doing so as progenitor requires the |
3 participants
Currently
reqwest::Clientis the only type we allow as the underlying HTTP client. Some consumers of Progenitor may want to usereqwest_middlewareto enable conveniences like automatic retries and support fortracing.Add a new
with_client_typeargument forGenerationSettingsto allow callers to select which of these types to use as the client.