-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Spawn local #1148
Spawn local #1148
Conversation
This is just a naive implementation. It seems it can be improved using a custom task queue, but that can be in a separate PR.
pub fn spawn_local<F>(future: F) | ||
where | ||
F: Future<Item = (), Error = ()> + 'static, | ||
{ |
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.
Is there a reason why only a F: Future<Item = (), Error = ()>
is allowed?
I would personally prefer
pub fn spawn_local(future: impl Future + 'static) {
future_to_promise(
future
.map(|_| JsValue::undefined())
.map_err(|_| JsValue::undefined()),
);
}
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 changed this to what you suggest :)
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.
@lcnr It is standard practice to require ()
. See tokio::run
and stdweb::spawn_local
.
The reason this is done is to prevent mistakes, and also to force the user to handle errors (rather than silently ignoring them, which is very bad!)
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.
Happy to do it either way. Maybe we should match other libraries though @lcnr. You can always make your own wrapper
pub fn spawn_local(future: impl Future + 'static) {
wasm_bindgen_futures::spawn_local(
future.map(|_| ()).map_err(|_| ())
)
}
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 thought about it and agree with @Pauan .
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.
woop consensus.
Are the docs good enough? It might be that the module documentation needs to be changed. |
Thanks @derekdreery! |
This is a very simple implementation of
spawn_local
, a function that spawns a rust future using the javascript promise infrastructure as the executor.