-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WASM: add abstraction for tokio::spawn
#2442
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved from a functional standpoint: web-client works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. And it even has a negative diff!
@@ -23,7 +23,7 @@ workspace = true | |||
async-trait = "0.1" | |||
futures = { package = "futures-util", version = "0.3" } | |||
futures-executor = { version = "0.3" } | |||
instant = { version = "0.1", features = [ "wasm-bindgen" ] } | |||
instant = { version = "0.1", features = ["wasm-bindgen"] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we would want to introduce a feature called wasm_bindgen
that enables this other feature such that it doesn't always get pulled. Something like:
[features]
wasm-bindgen = ["instant/wasm-bindgen"]
.expect("Client initialization failed"); | ||
let mut client = nimiq::client::Client::from_config(config) | ||
.await | ||
.expect("Client initialization failed"); | ||
log::info!("Web client initialized"); | ||
|
||
// Start consensus. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@styppo the web-client
crate has a bunch of wasm_bindgen::spawn_local
s. Those can get replaced with the new spawn
abstraction too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case it is guaranteed for it to not be a tokio::spawn
though as the web client will be compiled for the wasm target always. It could be done, but it would not add much I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's true but for consistency across the codebase using the abstraction as much makes sense. For example if you want to alter the impl behind the abstraction maybe later in the far future.
36e81ce
to
c1d996a
Compare
- Add a utility function `spawn` which calls `wasm_bindgen_futures::spawn_local` for WASM targets and `tokio::spawn` otherwise. - Remove `TaskExecutor`.
spawn
which callswasm_bindgen_futures::spawn_local
for WASM targets andtokio::spawn
otherwise.TaskExecutor
.Closes #2434.