reqwest-middleware icon indicating copy to clipboard operation
reqwest-middleware copied to clipboard

feat: wasm32-unknown-unknown support

Open Ekleog opened this issue 2 years ago • 2 comments

This replaces task-local-extensions with http's extensions, as http was already in the dependency closure anyway and the other features of task-local-extensions (that required an incompatible-with-wasm part of tokio) were not used anyway.

I tried it out in the project that triggered this PR, and it seems to work perfectly fine!

Ekleog avatar Dec 07 '22 01:12 Ekleog

Thanks for the PR! I had the same idea about http extensions in #77.

The extra wasm support code is definitely interesting. It's going to take me a bit to review it as I'm not an expert in wasm and reqwest

conradludgate avatar Dec 07 '22 11:12 conradludgate

:+1: To explain the wasm-related changes:

  • task-local-extensions requires tokio with the rt feature, which does not support the wasm async runtime
  • reqwest does not support the timeout function on wasm32, so I'm just cfg-ing it out
  • reqwest futures are not Send on wasm32, but wasm32 has a single thread anyway so we can just make the async-trait and BoxFuture !Send on wasm32
  • reqwest on wasm does not depend on hyper (which does not support wasm anyway), so I'm cfg-ing out the workaround for a reqwest limitation that requires a direct dependency on hyper from reqwest-middleware
  • tokio::time does not support wasm, so I'm using wasm-timer
  • reqwest on wasm32 does not have .is_connect() for the Error, so I'm cfg-ing the call out too (this is arguably an issue in reqwest upstream, that should have an is_connect() that always returns false)

Hopefully it can make the code review easier! :)

Ekleog avatar Dec 08 '22 14:12 Ekleog

Hey! Just checking if you had the time to have a look at my replies to your comments? :)

Ekleog avatar Jan 30 '23 16:01 Ekleog

Yeah, code wise I'm happy. I'm still going to have a think about future abstractions. Since this is a breaking change because it replaces the extensions type. Perhaps we could modify task-local-extensions to be a wrapper around http::Extensions and support wasm32 and be fully compatible

Maintenance wise, I'd like a way to test it to ensure we don't break things in future by forgetting attributes. Do you think you could come up with a mechanism to compile and run our tests on wasm in github CI?

conradludgate avatar Jan 30 '23 16:01 conradludgate

Makes sense, please let me know what your conclusions are about http::Extensions and task_local_extensions!

As for testing on wasm in github CI, unfortunately wasm-timer only supports wasm32-unknown-unknown (with web_sys support) and not wasm32-wasi (which is a very different platform). And widespread testing tools only support wasm32-wasi for now, which makes sense as otherwise one needs to actually spawn a browser to run tests. (To be precise, the webassembly-test crate can run tests for wasm32-unknown-unknown… but does not actually run them inside a browser)

So the best bet to run tests would I think be to use fantoccini and geckodriver to run a compiled-to-wasm version of the tests for this crate; maybe using webassembly-test to at least be able to reuse some infra of making it a test runner.

However, considering the small amount of code that is wasm32-specific after this PR, I don't think this is worth it, as the resulting codebase would probably end up larger than all of reqwest-middleware.

So maybe it'd make sense to just have CI compile to wasm32-unknown-unknown, and check that no regressions are made there? I've just added a commit that does that :)

Ekleog avatar Jan 30 '23 19:01 Ekleog

This has been released

conradludgate avatar Mar 09 '23 11:03 conradludgate