-
Notifications
You must be signed in to change notification settings - Fork 485
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(core): abstract HttpFetch trait for raw http client #5184
Conversation
core/src/raw/http_util/client.rs
Outdated
pub(crate) static GLOBAL_REQWEST_CLIENT: Lazy<reqwest::Client> = Lazy::new(reqwest::Client::new); | ||
|
||
/// HttpFetcher is a type erased [`HttpFetch`]. | ||
pub type HttpFetcher = Box<dyn HttpFetchDyn>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can use Arc<dyn HttpFetchDyn>
here to eliminate the need for an extra Box
.
} | ||
} | ||
|
||
pub trait HttpFetch: Send + Sync + Unpin + 'static { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add comments to this trait. This trait is exposed to users, and we expect that users should never interact with HttpFetchDyn
.
core/src/raw/http_util/client.rs
Outdated
} | ||
} | ||
|
||
impl<T: HttpFetchDyn + ?Sized> HttpFetch for Box<T> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implement for Arc<T>
instead.
@@ -25,6 +25,10 @@ | |||
mod client; | |||
pub use client::HttpClient; | |||
|
|||
/// temporary client used by several features | |||
#[allow(unused_imports)] | |||
pub(crate) use client::GLOBAL_REQWEST_CLIENT; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please make HttpFetch
public.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @everpcpc for working on this!
Which issue does this PR close?
Closes #4471
Rationale for this change
What changes are included in this PR?
Are there any user-facing changes?