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

[matheus23]

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

Old open questions, now resolved

• 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?

[djc]

Thanks, this looks like a reasonable direction!

Please squash all the changes into a single commit, as these commits are not conceptually separate.

[djc]

This is looking pretty good!

[matheus23]

Thanks for the review. Thoughts on this open question?

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

I'm on the fence. I think it may be better to depend on web_time on all targets. Otherwise I'll be repeating the same lib.rs changes in quinn itself (to avoid having quinn_proto::Duration be a public reexport).

[djc]

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

I'm on the fence. I think it may be better to depend on web_time on all targets. Otherwise I'll be repeating the same lib.rs changes in quinn itself (to avoid having quinn_proto::Duration be a public reexport).

I'm inclined to stick with depending on web_time only for WASM targets. IMO mainstream platforms should not bear the cost of niche platforms even if the overhead is small. @Ralith any thoughts?

[matheus23]

@djc @Ralith what's left to do here to get this over the finish line?
I'd love to also contribute quinn wasm32-unknown-unknown support after this.

[djc]

LGTM, please squash the commits?

[Ralith]

I'm inclined to stick with depending on web_time only for WASM targets. IMO mainstream platforms should not bear the cost of niche platforms even if the overhead is small. @Ralith any thoughts?

Given that the amount of extra code is pretty trivial, this stance seems reasonable to me. I know I'd be a bit confused as a non-web user to see a web crate show up in my environment.

[Ralith]

The wasm_bindgen_test churn is unfortunate, but I guess there's nothing for it.

[djc]

@matheus23 this is currently failing the WASM test, I guess that's not supposed to be intentional?

[matheus23]

Seems like there's potentially some ring issues with wasm32.
Investigating this. Also need to make sure that main doesn't have any commits added that regress wasm32 support (So I need to merge in main continuously).

[matheus23]

Okay, got to somewhat of the bottom on this. The stacktraces you saw were v8 crashing for some reason.
Updating nodejs from 18 to 20 fixes the issue.
Will update the PR now. There's also some tricks I learned along the way that make the diff nicer and the workflow faster.

[matheus23]

Also seems like cc version 1.1.32 broke the ring Wasm build (but in github actions only?).
I've submitted an upstream issue and forced version 1.1.31 in CI for now.

[matheus23]

FWIW, this PR is ready from my perspective. Is there anything left to do? Do you want to re-review?

[djc]

Thanks, this is looking good!