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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(wasm): add support for wasip2, aka webassembly components, using wasi-http #2290

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

brooksmtownsend
Copy link

@brooksmtownsend brooksmtownsend commented May 17, 2024

Fixes #2294

Hey there! This is a work-in-progress PR that adds support for using reqwest with wasi-http. It was really neat to see the support for wasm_bindgen and JavaScript bindings for requests, and this PR helps support the component model in addition to wasm_bindgen.

I wanted to open this PR to see if you had any suggestions, requests from the initial working code/refactor perspective, and if there's any additional material you'd like me to clarify. If this fits within your general contribution guidelines, I'll wrap it up and mark as ready for review!

This code is still very littered with unwraps and comments, so go easy on me if you take a look at the implementation 馃槄 . To name a few, here's the things I want to do still for this PR:

  • General code cleanliness, no doc comments simply added as dox, kill unwraps, expects, and panics
  • Add example that I'm using for testing under the examples dir
  • Gate wit-bindgen behind the wasm32 family of deps
  • See if I can better toggle between the JS/bindgen Wasm support and the component model wasm support using directives instead of a feature (there is the wasm32-wasip2 target but that's not very widely used yet.) The goal here is to not affect any existing use cases of wasm -> reqwest and add support for components.
  • Consider a custom pollable to remove the technically blocking code in the request

Last but not least, thanks for this library! Love reqwest 鉂わ笍

@seanmonstar
Copy link
Owner

Thanks for taking a look at this! I think support WASI is likely a good goal, in general. However, I usually try to have the discussion in an issue first. I find it's helpful to find consensus before spending too much time going down a certain implementation path, or even working on a feature that won't be accepted at all.

So while I do appreciate exploring the area, and exploration is often needed to inform more complex features, I do want to set proper expectations: we might not do this, or do it very differently. 馃檲

@brooksmtownsend
Copy link
Author

@seanmonstar that's why I opened the draft PR! Glad I ended up doing that before I got too far down the road. I can open up an issue then and point at this branch as a reference if that's what you prefer

@brooksmtownsend
Copy link
Author

brooksmtownsend commented May 17, 2024

Oh fun, #1956 and #1977 exist. Do we need another issue? Looks like the implementation in #1956 is also an option for comparison, albeit not quite on wasi-http@0.2.0.

@brooksmtownsend
Copy link
Author

Created #2294 to further the discussion, @seanmonstar please feel free to either close this PR for now while we continue in the issue or leave it open, totally up to the way you like to track the OSS work.

@brooksmtownsend brooksmtownsend force-pushed the feat/wasi-p2-component-support branch 2 times, most recently from 7a17bbe to f623025 Compare May 22, 2024 16:33
@brooksmtownsend
Copy link
Author

brooksmtownsend commented May 22, 2024

Rebased this PR off of main and added a component example so folks can see this in action. With reqwest it is really nice looking

Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>

refactor(wasm): add wasm mod

Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>

wip: feat(wasm): add component feature

Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
Signed-off-by: Brooks Townsend <brooksmtownsend@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support WebAssembly Component bindings, aka wasip2, in addition to javascript bindings
2 participants