quinn icon indicating copy to clipboard operation
quinn copied to clipboard

feat(quinn-proto): Make compilable in wasm32-unknown-unknown and run wasm tests in CI

Open matheus23 opened this issue 1 year ago • 2 comments

Summary of Changes

  • Use web_time dependency instead of std::time & re-export Instant, Duration, SystemTime and UNIX_EPOCH in lib.rs so imports are simpler
  • Adjust imports to use crate::{Instant, Duration, ...} instead of std::time::{Instant, Duration, ...}
  • Added wasm-bindgen-test to enable running tests from a Wasm build
  • Added a CI task to test quinn-proto tests in Wasm with node.js

Running quinn-proto tests in Wasm

# install wasm-bindgen-test-runner
$ cargo install wasm-bindgen-cli
# run tests
$ cargo test -p quinn-proto --target wasm32-unknown-unknown

Notes & Open Questions

  • Should the added code/dependencies additionally be feature-gated?

    I personally don't think so. As far as I know, there's practically no one asking for wasm32-unknown-unknown compilability who isn't targeting nodejs/browsers/cloudflare workers/etc. and in all of these cases wasm-bindgen is the de-facto standard. There's also targeting wasmtime and other runtimes, but IMO those needs are met by wasm32-wasi-* targets, not wasm32-unknown-unknown.

    That said, for my intents and purposes that's mostly a cosmetic question, I don't mind having to enable another feature flag. Other concerns could be "what non-web wasm32-unknown-unknown becomes a thing and we need to break compatibility by introducing features?".

  • Should the Duration, Instant, etc. re-exports live in lib.rs?

    They're pub(crate) only, so I don't think it matters much. I can move them into a file if you want, or we keep them here.

  • Should we just depend on web_time instead of std::time everywhere instead of cfg-gating & re-exporting?

    web_time itself already multiplexes between std::time and a web-based implementation. So in a way, we're doing it twice here. What we gain is not depending on web_time most of the time. However, once we work on Wasm support for e.g. quinn itself, we'd need to do this whole re-export dance, again. So perhaps it'd be better to just depend on web_time everywhere?

matheus23 avatar Sep 26 '24 10:09 matheus23