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

feat: wasm32-unknown-unknown support #79

Merged
merged 7 commits into from
Mar 9, 2023
Merged

Conversation

Ekleog
Copy link
Contributor

@Ekleog Ekleog commented Dec 7, 2022

Makes a bunch of stuff not require Send, since reqwest on wasm32 produces non-send futures.

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

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.
@Ekleog Ekleog requested a review from a team as a code owner December 7, 2022 01:11
@conradludgate
Copy link
Collaborator

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

@Ekleog
Copy link
Contributor Author

Ekleog commented Dec 8, 2022

👍 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
Copy link
Contributor Author

Ekleog commented Jan 30, 2023

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

@conradludgate
Copy link
Collaborator

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?

@Ekleog
Copy link
Contributor Author

Ekleog commented Jan 30, 2023

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 :)

conradludgate
conradludgate previously approved these changes Mar 9, 2023
Copy link
Collaborator

@conradludgate conradludgate left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience. I've managed to make task-local-extensions compile without tokio-rt so I've kept that as is for now. This means we can release this as a non-breaking change!

@conradludgate conradludgate merged commit fef18b3 into TrueLayer:main Mar 9, 2023
@conradludgate
Copy link
Collaborator

This has been released

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants