schell / mogwai

The minimalist, obvious, graphical, web application interface
429 stars 26 forks source link

Change `Transmitter::send_async` only exist for wasm32 #58

Closed bryanjswift closed 4 years ago

bryanjswift commented 4 years ago

The futures::executor::block_on didn't work inside a tokio runtime which will be too common a scenario to ignore. Outside of implementing a custom Waker to loop and poll the Future to completion spawning a "local" context similar to the wasm32 behavior is more difficult than anticipated. Without the use of futures in Transmitter::send_async it is safe to remove futures from the dependencies of mogwai.

Refs #52 Closes #57

bryanjswift commented 4 years ago

Simply removing the implementation for #[cfg(not(target_arch = "wasm32"))] might be better. It would certainly make it more obvious to consumers of the crate that the feature was only available for wasm32 and would maybe prevent surprising runtime behavior 🤔

schell commented 4 years ago

I support removing it, but there's a catch: we need it for the docs, which is compiled w/ target_arch = "x86_64", or similar depending on the machine running it. Maybe we can mark it with #[deprecated] and then panic inside the impl?

bryanjswift commented 4 years ago

I support removing it, but there's a catch: we need it for the docs, which is compiled w/ target_arch = "x86_64", or similar depending on the machine running it. Maybe we can mark it with #[deprecated] and then panic inside the impl?

I had originally had the contents as unimplemented!("Transmitter::send_asyncis only implemented for wasm32") which does panic. I didn't like the feel of having an intentional runtime panic in the published API though (I suppose a runtime do nothing is arguably worse though so... there's that). Is it possible to compile docs with a different target_arch?

bryanjswift commented 4 years ago

#[cfg(doc)] appears to work for allowing cargo doc to run, cargo test fails to find it with #[cfg(test)] though. Trying some other options.