-
-
Notifications
You must be signed in to change notification settings - Fork 272
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
Wasm build by adding default-client
feature gate
#591
Conversation
@@ -265,6 +263,7 @@ pub type Result<T, E = error::Error> = std::result::Result<T, E>; | |||
|
|||
const GITHUB_BASE_URI: &str = "https://api.github.com"; | |||
|
|||
#[cfg(feature = "default-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.
What makes this one incompatible with WASM?
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.
It depends on OctocrabBuilder::default().build()
, which is not available on wasm32. Another approach would be having 2 different OctocrabBuilder::default()
impls.
I also found another issue with building for wasm: Buffer::new()
here depends on tokio. To make it work on wasm I had to changed to Buffer::pair
, pass in the executor and spawn the worker task.
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.
Yeah, I think I'd prefer having default client that works on WASM.
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.
I re-checked the code and realized why I didn't go with 2 default impls in the first place: there's currently no good default Svc
on wasm. An empty/do-nothing one would be pointless here, since the STATIC_INSTANCE
would not work anyway. Bringing in a working Svc
for wasm looks like the better option, and reqwest
is a great candidate. However it's still using an old version of http
seanmonstar/reqwest#2059
I made a half-working implementation using gloo_net
, however it involves manually construct gloo_net::Request
from http::Request<String>
, making the code ugly. Soon after testing a few endpoint, I realize the github rest API would not work for my use case, so had to switch to its graphql API, so I didn't finish the wasm http client tower::Service
implementation.
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.
Yeah, reqwest
is a no go, because it does not support using or getting http::{Request, Response}
, reqwest
returns its own types and in order to be Sans-IO and client agnostic, octocrab
has to support http
types, not reqwest
types.
This enables wasm build by adding a new
default-client
feature and putting all wasm incompatible code behind it.On wasm targets users can supply their own
Service<Request<String>, Response = Response<B>> + Send + 'static
.Fixes #366 #224