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

Allow users to pass in a timeout for the API client #45

Closed
gabbifish opened this issue Sep 30, 2019 · 6 comments · Fixed by #46
Closed

Allow users to pass in a timeout for the API client #45

gabbifish opened this issue Sep 30, 2019 · 6 comments · Fixed by #46
Labels
enhancement New feature or request

Comments

@gabbifish
Copy link
Contributor

This is related to cloudflare/wrangler-legacy#746.

Ideally, users should be allowed to specify the API client timeout period when constructing the API client with HttpApiClient::new(). What do you think?

@gabbifish gabbifish added the enhancement New feature or request label Sep 30, 2019
@adamchalmers
Copy link
Contributor

I agree!

@gabbifish
Copy link
Contributor Author

gabbifish commented Sep 30, 2019

Thinking about the maintainability of this in the long term... perhaps we want to make the HttpApiClient 's http_client field mutable, so a user of cloudflare-rs can customize this client field themselves however they see fit using https://docs.rs/reqwest/0.8.4/reqwest/struct.ClientBuilder.html. If we move a timeout param into the Http::ApiClient::new() function and build a client there, I do wonder if that will be the beginning of bloating new() with new param arguments.

I think I prefer having the http_api field in HttpApiClient become mut so users can provide their own reqwest::Client with all their custom configs there. What do you think?

@gabbifish
Copy link
Contributor Author

gabbifish commented Sep 30, 2019

or, we can even implement HttpApiClient::from(reqwest_client), where reqwest_client is the custom one built by the user?

@ashleymichal
Copy link
Contributor

ashleymichal commented Sep 30, 2019

fwiw, i personally prefer a constructor argument to a mutable struct field. generally speaking our client is not a state machine; it is instantiated to do a job, is called to do that job, and then is dropped. I'd rather have the whole configuration passed at construction.

@adamchalmers
Copy link
Contributor

100% agree with Ashley

@gabbifish
Copy link
Contributor Author

gabbifish commented Oct 1, 2019

Alright, I'll get going with constructor argument then! :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants