Support for wasm32-unknown-unknown

[expenses]

Hi. While I know that this crate is meant to be super minimal, it's super, super, super useful to be able to use it in wasm. This PR adds that feature with no added dependencies for the native version. The implementation of global_wasm.rs, written by @tomaka, is a little hacky, so that could perhaps be improved a little.

[yoshuawuyts]

Hi, thanks for opening this! I'm excited for making futures-timer work in the browser. I've left a few comments that I'd like to see addressed.

Also paging @tomaka who has done a lot of work in this area already. Do you have any opinions as to how we could best approach this?

[tomaka]

You don't have to rename the crate. wasm-bindgen can be the feature itself.

[expenses]

How do I make sure the other dependencies are enabled if I do that?

[tomaka]

Suggested change

	wasm-bindgen-crate = { package = "wasm-bindgen", version = "0.2.55", optional = true }
	wasm-bindgen = { version = "0.2.55", optional = true }

Only this change ⬆️ is needed. The content of your [features] section below will still work.

[expenses]

Are you sure? Features and dependencies cannot have the same name: 'wasm-bindgen'

[tomaka]

I would have sworn this was working, so I did a bit of research and apparently I think that used to be possible but wasn't intended.

[tomaka]

Also paging @tomaka who has done a lot of work in this area already. Do you have any opinions as to how we could best approach this?

I agree with you in that it should be feature-gated behind a wasm-bindgen feature.
(this PR has since then been updated to do that)

[tomaka]

The code (that I wrote) is also extremely hacky. It will leak pretty badly, but it's not easily fixable without rethinking the approach.

[expenses]

@yoshuawuyts are you happy with these changes?

[tomaka]

I'm not super happy with these changes.

One fundamental issue with making the futures-timer crate compatible with wasm32-unknown-unknown is that it exposes Instant in its API, which is a dummy implementation in the standard library.

Ideally, futures-timer::Delay would be just new(Duration) and that's it.

Similarly, we should ideally just map futures-timer::Delay to one call to setTimeout, rather than using this wheel timer thingy on WASM.

[yoshuawuyts]

@expenses I'm happy to follow @tomaka's lead on the suggestions here.

[tomaka]

After chatting a bit with @expenses and brainstorming a bit, what we should do IMO is:

• If compiling for wasm32-unknown-unknown and without the wasm-bindgen feature, then we use the default existing code, which will panic here:

  futures-timer/src/delay.rs

  Line 38 in 4586461

  Delay::new_handle(Instant::now() + dur, Default::default())

This sucks, but it's the least bad solution to me.

• If compiling for wasm32-unknown-unknown and with the wasm-bindgen feature, then we use this code here (except that the parameter would be a Duration rather than a number of milliseconds), either by depending on gloo-timers or by copy-pasting its code (it's very small).

• If compiling for wasm32-unknown-unknown and with the wasm-bindgen feature, there's still the problem of having methods that accept an Instant. Considering that it's impossible for the user to build a std::instant::Instant on wasm-unknown, I think we can implement them as panics.

[yoshuawuyts]

@tomaka overall that sounds very reasonable!

--

I think we can implement them as panics.

Two thoughts:

1. The when method exists pretty much only to implement stream::Interval in async_std. Perhaps if we moved the interval code back into this crate we could provide a WASM version as well, and remove the when method so it can never panic?
2. The reset method could perhaps be modified to take a Duration instead, so it can work on both platforms. This is how the old reset method used to work too.

Most of these changes were made without knowing that Instant is not available in the browser. Perhaps by changing the APIs we can perhaps ensure this crate never panics, and all parts of it work cross-platform?

[tomaka]

If we're allowed breaking changes, then I'm all in favour of doing that!

[yoshuawuyts]

@tomaka heh, yeah we totally can. Would be a major version, but that's what they're for (:

[expenses]

@tomaka ready for review again. I'm not sure what the best thing to do if Delay::reset get called is. It would be possible to reset the timer if we we used wasm-timer Instants, but we're not at the moment.

[expenses]

@tomaka, @yoshuawuyts sorry for the delay, I failed to see these comments before.

[tomaka]

LGTM in general.

[yoshuawuyts]

Looks good overall; few nits but otherwise should be good to merge!

edit: also clippy seems to be failing.

[expenses]

Clippy is still failing but if the tests panic if I change mem::forget(me.clone()); to mem::forget(me);

[yoshuawuyts]

@expenses odd; yeah maybe we should just disable clippy. I think this is good to merge as-is. Thanks!

[expenses]

@yoshuawuyts awesome, thanks!! lemme buy you a beer sometime ;^)

[yoshuawuyts]

@expenses published v3.0.0 🎉