Support object_store with wasm: Default wasm32-unknown-unknown HttpConnector#329
Conversation
alamb
left a comment
There was a problem hiding this comment.
Thank you @H-Plus-Time and @kylebarron -- this looks really neat
I think it is important to have some sort of test for this feature otherwise it is quite likely it will be broken in some future reactor
How did you test this locally? Perhaps we can adapt such a test to run in our CI?
|
I tested through object-store-wasm (which provided all the wasm-bindgen bindings to create object store instances and so on from JS), but a reasonable approach would be:
wasm-bindgen-test essentially does 1 and 3, a simple startup and teardown script for the server component would be all that's needed for 2. It shouldn't be too much trouble, I'll get something basic together in the next few days. |
|
Thanks @H-Plus-Time 🙏 (Note I will be out next week so I may be delated in responding) |
…2, as well as those that use the MockServer. Very basic test wasm tests, address dropped/unused receiver in wasm bridge code
|
That should do it I think - that one test covers a sizable chunk of the wasm-specific code; put/post requests would be the only other aspect worth testing I think. Oh, and I'm a little circumspect about including the |
| run: cargo build --all-features --target wasm32-wasip1 | ||
| - name: Install wasm-pack | ||
| run: curl https://rustwasm.github.io/wasm-pack/installer/init.sh -sSf | sh | ||
| - name: Run wasm32-unknown-unknown tests (via Node) |
|
kicked off the tests |
|
Looks like there are some CI failures: https://github.com/apache/arrow-rs-object-store/actions/runs/14495449031/job/40672770867?pr=329 |
alamb
left a comment
There was a problem hiding this comment.
Looks good to me -- thank you very much @H-Plus-Time
|
|
||
| #[cfg(all(feature = "http", target_arch = "wasm32", target_os = "unknown"))] | ||
| #[wasm_bindgen_test] | ||
| async fn basic_wasm_get() { |
There was a problem hiding this comment.
I verified this runs in CI: https://github.com/apache/arrow-rs-object-store/actions/runs/14505171194/job/40723998384?pr=329#step:11:138
warning: `object_store` (lib) generated 1 warning
warning: `object_store` (lib test) generated 4 warnings (run `cargo fix --lib -p object_store --tests` to apply 3 suggestions)
Finished `test` profile [unoptimized + debuginfo] target(s) in 0.11s
Running unittests src/lib.rs (target/wasm32-unknown-unknown/debug/deps/object_store-4bf7d6aa80f96256.wasm)
no tests to run!
Running tests/http.rs (target/wasm32-unknown-unknown/debug/deps/http-0715721c2cebdfb7.wasm)
Executing bindgen...
running 1 test
test basic_wasm_get ... ok
test result: ok. 1 passed; 0 failed; 0 ignored; 0 filtered out; finished in 0.36s
So looks good to me
|
FYI @kylebarron could you perhaps take a look at this? Also, FYI @XiangpengHao |
object_store with wasm: Default wasm32-unknown-unknown HttpConnector
kylebarron
left a comment
There was a problem hiding this comment.
Channels always confuse me, but overall looks good.
| use futures::{ | ||
| channel::{mpsc, oneshot}, | ||
| SinkExt, StreamExt, TryStreamExt, | ||
| }; | ||
| use http_body_util::{Empty, StreamBody}; | ||
| use wasm_bindgen_futures::spawn_local; |
There was a problem hiding this comment.
Nit: move imports to the top of this scope? E.g. before the req.into_parts line?
There was a problem hiding this comment.
Ah, yep - I must have started from a copy-paste of the non-wasm impl, I'll move them.
|
Thanks again everyone. From my perspective this PR is ready to merge but I want to leave it open for another day or two to allow more time for comments. I'll be away next week so if another committer doesn't merge it before I'll merge it when I get back |
|
Thank you for this, looks good to me 👍 |
…Connector (apache#329) * Implement default wasm32-unknown-unknown (JS platform dependent) HttpConnector. * Disable all test blocks that strictly require the fs feature on wasm32, as well as those that use the MockServer. Very basic test wasm tests, address dropped/unused receiver in wasm bridge code * CI wasm32-unknown-unknown tests * Install node for wasm tests in CI * cargo fmt * Hoist use/imports in wasm HttpService
Which issue does this PR close?
object_storewith wasm: Default WASM32 HttpConnector arrow-rs#7227.What changes are included in this PR?
Target-gated wasm32-unknown-unknown reqwest-based HttpConnector (with explicit handling surrounding non-Send, non-Sync futures).
Are there any user-facing changes?
wasm32-unknown-unknown users will need to add the
getrandom = { version = "*", features = ["js"] }cargo entry if they aren't already using it. The alternative would be to feature flag in a similar manner to chrono, getrandom, zstd (i.e. 'js').