XAMPPRocky / octocrab

A modern, extensible GitHub API Client for Rust.
Other
1.06k stars 259 forks source link

Octocrab doesn't compile with Yew (for me) #224

Open NicMcPhee opened 2 years ago

NicMcPhee commented 2 years ago

I was hoping to use Octocrab on a Yew-based project that I'm working on, but when I added the octocrab dependency to my Cargo.toml file, octocrab failed to compile.

I created a small demo app that illustrates the issue: https://github.com/NicMcPhee/octocrab_test This is about the simplest possible Yew app, and if you uncomment the octocrab dependency in Cargo.toml and run trunk serve, you get a whole pile of compilation errors when it's trying to build the octocrab code (see below).

I'm guessing it's some kind of dependency conflict, but I really don't know.

Yew + Octocrab seems like an "obvious" combination, so it was definitely a bummer that this didn't work. Any suggestions or advice would definitely be appreciated.

Rust versions, etc.

rustup show provides the following output if that's helpful:

Default host: x86_64-apple-darwin
rustup home:  /Users/mcphee/.rustup

installed targets for active toolchain
--------------------------------------

wasm32-unknown-unknown
x86_64-apple-darwin

active toolchain
----------------

stable-x86_64-apple-darwin (default)
rustc 1.62.0 (a8314ef7d 2022-06-27)

The error text

   Compiling octocrab v0.16.0
error[E0599]: no method named `user_agent` found for struct `ClientBuilder` in the current scope
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/lib.rs:338:14
    |
338 |             .user_agent("octocrab")
    |              ^^^^^^^^^^ method not found in `ClientBuilder`

error[E0599]: no method named `user_agent` found for struct `ClientBuilder` in the current scope
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/lib.rs:432:18
    |
432 |                 .user_agent("octocrab")
    |                  ^^^^^^^^^^ method not found in `ClientBuilder`

error: future cannot be sent between threads safely
  --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/from_response.rs:11:80
   |
11 |       async fn from_response(response: reqwest::Response) -> crate::Result<Self> {
   |  ________________________________________________________________________________^
12 | |         let text = response.text().await.context(crate::error::HttpSnafu)?;
13 | |
14 | |         let de = &mut serde_json::Deserializer::from_str(&text);
15 | |         serde_path_to_error::deserialize(de).context(crate::error::JsonSnafu)
16 | |     }
   | |_____^ future created by async block is not `Send`
   |
   = help: within `impl Future<Output = std::result::Result<T, error::Error>>`, the trait `Send` is not implemented for `*mut u8`
note: captured value is not `Send`
  --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/from_response.rs:11:28
   |
11 |     async fn from_response(response: reqwest::Response) -> crate::Result<Self> {
   |                            ^^^^^^^^ has type `Response` which is not `Send`
   = note: required for the cast to the object type `dyn Future<Output = std::result::Result<T, error::Error>> + Send`

error: future cannot be sent between threads safely
  --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/from_response.rs:11:80
   |
11 |       async fn from_response(response: reqwest::Response) -> crate::Result<Self> {
   |  ________________________________________________________________________________^
12 | |         let text = response.text().await.context(crate::error::HttpSnafu)?;
13 | |
14 | |         let de = &mut serde_json::Deserializer::from_str(&text);
15 | |         serde_path_to_error::deserialize(de).context(crate::error::JsonSnafu)
16 | |     }
   | |_____^ future created by async block is not `Send`
   |
   = help: within `impl Future<Output = std::result::Result<T, error::Error>>`, the trait `Send` is not implemented for `Rc<RefCell<wasm_bindgen_futures::Inner>>`
note: future is not `Send` as this value is used across an await
  --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/reqwest-0.11.11/src/wasm/mod.rs:22:41
   |
22 |     let js_val = JsFuture::from(promise).await.map_err(crate::error::wasm)?;
   |                  -----------------------^^^^^^ await occurs here, with `JsFuture::from(promise)` maybe used later
   |                  |
   |                  has type `wasm_bindgen_futures::JsFuture` which is not `Send`
note: `JsFuture::from(promise)` is later dropped here
  --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/reqwest-0.11.11/src/wasm/mod.rs:22:76
   |
22 |     let js_val = JsFuture::from(promise).await.map_err(crate::error::wasm)?;
   |                                                                            ^
   = note: required for the cast to the object type `dyn Future<Output = std::result::Result<T, error::Error>> + Send`

error: future cannot be sent between threads safely
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/page.rs:88:80
    |
88  |       async fn from_response(response: reqwest::Response) -> crate::Result<Self> {
    |  ________________________________________________________________________________^
89  | |         let HeaderLinks {
90  | |             first,
91  | |             prev,
...   |
126 | |         }
127 | |     }
    | |_____^ future created by async block is not `Send`
    |
    = help: within `impl Future<Output = std::result::Result<page::Page<T>, error::Error>>`, the trait `Send` is not implemented for `*mut u8`
note: captured value is not `Send`
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/page.rs:88:28
    |
88  |     async fn from_response(response: reqwest::Response) -> crate::Result<Self> {
    |                            ^^^^^^^^ has type `Response` which is not `Send`
    = note: required for the cast to the object type `dyn Future<Output = std::result::Result<page::Page<T>, error::Error>> + Send`

error: future cannot be sent between threads safely
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/page.rs:88:80
    |
88  |       async fn from_response(response: reqwest::Response) -> crate::Result<Self> {
    |  ________________________________________________________________________________^
89  | |         let HeaderLinks {
90  | |             first,
91  | |             prev,
...   |
126 | |         }
127 | |     }
    | |_____^ future created by async block is not `Send`
    |
    = help: within `impl Future<Output = std::result::Result<page::Page<T>, error::Error>>`, the trait `Send` is not implemented for `Rc<RefCell<wasm_bindgen_futures::Inner>>`
note: future is not `Send` as this value is used across an await
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/reqwest-0.11.11/src/wasm/mod.rs:22:41
    |
22  |     let js_val = JsFuture::from(promise).await.map_err(crate::error::wasm)?;
    |                  -----------------------^^^^^^ await occurs here, with `JsFuture::from(promise)` maybe used later
    |                  |
    |                  has type `wasm_bindgen_futures::JsFuture` which is not `Send`
note: `JsFuture::from(promise)` is later dropped here
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/reqwest-0.11.11/src/wasm/mod.rs:22:76
    |
22  |     let js_val = JsFuture::from(promise).await.map_err(crate::error::wasm)?;
    |                                                                            ^
    = note: required for the cast to the object type `dyn Future<Output = std::result::Result<page::Page<T>, error::Error>> + Send`

error: future cannot be sent between threads safely
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/models/repos.rs:119:80
    |
119 |       async fn from_response(response: reqwest::Response) -> crate::Result<Self> {
    |  ________________________________________________________________________________^
120 | |         let json: serde_json::Value = response.json().await.context(crate::error::HttpSnafu)?;
121 | |
122 | |         if json.is_array() {
...   |
132 | |         }
133 | |     }
    | |_____^ future created by async block is not `Send`
    |
    = help: within `impl Future<Output = std::result::Result<ContentItems, error::Error>>`, the trait `Send` is not implemented for `*mut u8`
note: captured value is not `Send`
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/models/repos.rs:119:28
    |
119 |     async fn from_response(response: reqwest::Response) -> crate::Result<Self> {
    |                            ^^^^^^^^ has type `Response` which is not `Send`
    = note: required for the cast to the object type `dyn Future<Output = std::result::Result<ContentItems, error::Error>> + Send`

error: future cannot be sent between threads safely
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/octocrab-0.16.0/src/models/repos.rs:119:80
    |
119 |       async fn from_response(response: reqwest::Response) -> crate::Result<Self> {
    |  ________________________________________________________________________________^
120 | |         let json: serde_json::Value = response.json().await.context(crate::error::HttpSnafu)?;
121 | |
122 | |         if json.is_array() {
...   |
132 | |         }
133 | |     }
    | |_____^ future created by async block is not `Send`
    |
    = help: within `impl Future<Output = std::result::Result<ContentItems, error::Error>>`, the trait `Send` is not implemented for `Rc<RefCell<wasm_bindgen_futures::Inner>>`
note: future is not `Send` as this value is used across an await
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/reqwest-0.11.11/src/wasm/mod.rs:22:41
    |
22  |     let js_val = JsFuture::from(promise).await.map_err(crate::error::wasm)?;
    |                  -----------------------^^^^^^ await occurs here, with `JsFuture::from(promise)` maybe used later
    |                  |
    |                  has type `wasm_bindgen_futures::JsFuture` which is not `Send`
note: `JsFuture::from(promise)` is later dropped here
   --> /Users/mcphee/.cargo/registry/src/github.com-1ecc6299db9ec823/reqwest-0.11.11/src/wasm/mod.rs:22:76
    |
22  |     let js_val = JsFuture::from(promise).await.map_err(crate::error::wasm)?;
    |                                                                            ^
    = note: required for the cast to the object type `dyn Future<Output = std::result::Result<ContentItems, error::Error>> + Send`
XAMPPRocky commented 2 years ago

Thank you for your issue! I'm afraid AFAICT, there's not much that I can do to fix this issue, as it seems to be an issue with reqwest's futures. I'm not the one storing a *mut u8 or Rc<RefCell<wasm_bindgen_futures::Inner>> in the future. I agree with you that these should work together, which is why I eventually want to remove reqwest as a dependency (see #99), so people can use their clients and even if reqwest doesn't work in your environment, you can use another HTTP client that does.

NicMcPhee commented 2 years ago

Thanks for the prompt response! I'm sad that Octocrab and Yew aren't gonna be friends for a bit, but I get that it's not always easy to get different tools to play nice together.

I'm pretty new to Rust and am just working on my project as a way to learning Rust. I have a lot of programming experience, but more in spaces like Java and Clojure. How hard do you feel the migration away from reqwest will be? Is that something that I could contribute to in a meaningful way? Or should I just try something else like Octorust?

Thanks for your thoughts and work!

XAMPPRocky commented 2 years ago

Is that something that I could contribute to in a meaningful way? Or should I just try something else like Octorust?

Absolutely, contributions are always welcome and encouraged.

How hard do you feel the migration away from reqwest will be?

It's fairly straightforward in terms of complexity, as all we're doing is replacing request::Client with Arc<dyn tower::Service<http::Request, Response=http::Response, Error=Box<dyn std::error::Error>>, and migrating the methods to use that. It just needs someone to dedicate the time which I haven't been able to.

If you want to take this, and you get stuck, or don't understand something, please feel free to post in the issue and I'll try my best to help. I'd recommend looking at kube-client's code if you need something to start from, as we essentially want the same thing, but for Octocrab.

NicMcPhee commented 2 years ago

Thanks – I might have a look at it, but no promises for anything (fast). 😜

In #99 you suggest trying to address this one endpoint or group at a time. Any suggestions on a good "simple" endpoint to start with just so I can convince myself I understand the issues at hand?

XAMPPRocky commented 2 years ago

They’re all roughly the same complexity because the code is very simple. Thinking about it now, I would maybe take a different approach of just changing the HTTP methods (e.g. Octocrab::get and Octocrab::_get) and that should make it work for all endpoints.

NicMcPhee commented 2 years ago

Looking at lib.rs, it seems like everything ultimately goes through Octocrab::execute. So one option would be to just rewrite execute to use http, tower, etc., and then chase all the bazillion resulting compiler errors until everything works again. That would get everything done, but I could imagine that it might bleed change across fairly large chunks of the project and end up being a pretty big PR.

Another option would perhaps be to add a new http_execute function (or some other name as you prefer) that returns the non-reqwest response type. We could then deprecate the existing execute(), and work through the dependencies in a more one-at-a-time fashion.

Thoughts? Preferences?

Carreau commented 2 years ago

I have a similar issue trying to compile to wasm32-unknown-unknown for use in cloudflare workers – also learning rust. Though I am mostly interested only using the structs in octocrab::models. Is there any way to feature-gate reqwest/FromResponse, or only compile only the the structs ?

Maybe make the structs themselves part of a different module/crate that can be compiled separately ? Sorry not too quite sure what it possible.

MRuecklCC commented 1 year ago

There is also surf which abstracts different HTTP client implementations behind a common interface. It claims to support wasm environment.

IgnisDa commented 1 year ago

@XAMPPRocky Now that #297 has been merged, I think it might be possible to support wasm32-unknown-unknown target. Would be great if this was possible.

Right now having octocrab = { git = "https://github.com/XAMPPRocky/octocrab.git", rev = "dd02ea", default-features = false } as a dependency gives a lot of errors:

image

The main problem is the mio dependency.

XAMPPRocky commented 1 year ago

The main problem is the mio dependency.

That seems like a bug, we don't use mio directly, so a dependency we're using is incorrectly depending on mio for wasm.

martinitus commented 1 year ago

I was playing around with a similar idea last year. I think to make that work, one would have to implement the tower::Service<Request<B>> based on the websys crate. I had a look at gloo-net which does ship its own client implementation based on websys (here: https://github.com/rustwasm/gloo/blob/master/crates/net/src/http/mod.rs). It might be a good starting point. Essentially if such an implementation would exist, this would solve the same problem for all kinds of API client libraries (not only octocrab). I even asked in gloo/wasm/tower discord, there seems to be no such thing right now and i was encouraged to give it a try. Maybe you can ask there again :)

IgnisDa commented 1 year ago

So right now, it is still not possible to use octocrab with wasm32-unknown-unknown right?

Tehnix commented 1 month ago

Since https://github.com/XAMPPRocky/octocrab/pull/591 brought in the feature flag default-client, we can now avoid the parts that are incompatible with WASM.

E.g.

# Disable default features since default-client is incompatible with WASM targets.
octocrab = { version = "0.39.0", default-features = false, features = [
  "follow-redirect",
  "retry",
  "rustls",
  "timeout",
  "tracing",
] }

However, now we'll need to construct a compatible client that can be used in place of the original default client.

I more of less got as far as:

    let octocrab = octocrab::OctocrabBuilder::new_empty()
        .with_service(client)
        .with_layer(&BaseUriLayer::new(Uri::from_static(
            "https://api.github.com",
        )))
        .with_layer(&ExtraHeadersLayer::new(Arc::new(vec![(
            USER_AGENT,
            "octocrab".parse().unwrap(),
        )])))
        .with_auth(octocrab::auth::Auth::PersonalToken(
            api_token.to_owned().into(),
        ))
        .build()?;

but still need something to plug in as the Client, and have been failing to find a good solution so far 😅

Has anyone succeeded in this for WASM targets?