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
Introduce a new use_async hook, that allows for setting dependencies #3609
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
use std::{fmt, future::Future}; | ||
|
||
use crate::{functional::hook, use_effect_with, use_state, UseStateHandle}; | ||
|
||
#[hook] | ||
pub fn use_async<Arg, F, Fut, Res>(deps: Arg, f: F) -> UseAsyncHandle<Res> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The convention with dependencies is to have two hooks, |
||
where | ||
Arg: 'static + PartialEq, | ||
F: 'static + FnOnce(&Arg) -> Fut, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure if If you agree, please rewrite, and remove the |
||
Fut: 'static + Future<Output = Res>, | ||
Res: 'static, | ||
{ | ||
let result = use_state(|| None); | ||
use_effect_with(deps, { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. By making this hook an effect, its potentially only useable on the client side, as effects don't run in SSR. Is this intended? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hmm so TBH I have very little knowledge of how SSR is supposed to work, and zero experience with it — I'm using yew to develop cross-platform pseudo-heavy applications rather than lightweight webapps. Do you see EDIT: NVM, I just saw There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was thinking that perhaps you want to asynchronously fetch data on the server to render. But on second thought, this was a bit hasty because the component will be only rendered once in SSR anyway, so any resolved result will likely never make it into the component anyway if this works just similar to Note, because you also mention it in the other comment about |
||
let result = result.clone(); | ||
move |deps| { | ||
result.set(None); | ||
let future = f(deps); | ||
wasm_bindgen_futures::spawn_local(async move { | ||
Ekleog marked this conversation as resolved.
Show resolved
Hide resolved
|
||
let res = future.await; | ||
result.set(Some(res)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unconditionally setting the result means that futures from earlier renders, that happened to resolve later can override an existing result with potentially stale date. I don't think that's a good idea. Consider the following sequence:
|
||
}); | ||
} | ||
}); | ||
UseAsyncHandle { inner: result } | ||
} | ||
|
||
#[derive(Clone, PartialEq)] | ||
pub struct UseAsyncHandle<T> { | ||
inner: UseStateHandle<Option<T>>, | ||
} | ||
|
||
#[derive(Debug)] | ||
pub enum UseAsyncResult<'a, T> { | ||
Pending, | ||
Ready(&'a T), | ||
} | ||
|
||
impl<T: fmt::Debug> fmt::Debug for UseAsyncHandle<T> { | ||
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
let inner_value: &Option<T> = &*self.inner; | ||
match inner_value { | ||
None => f.write_str("Pending"), | ||
Some(val) => f.debug_tuple("Ready").field(val).finish(), | ||
} | ||
} | ||
} | ||
|
||
impl<T> UseAsyncHandle<T> { | ||
pub fn status(&self) -> UseAsyncResult<'_, T> { | ||
match &*self.inner { | ||
None => UseAsyncResult::Pending, | ||
Some(res) => UseAsyncResult::Ready(res), | ||
} | ||
} | ||
} |
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.
Needs documentation. In particular, this should mention
use_future
and when to use which of the two. Feel free to also adjust the docs there to point back here so that someone stumbling upon either can decide which one to use. And ask if you're not sure aboutuse_future
yourself.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.
Ok so now I feel stupid. I had looked into
yew::functional
for hooks, and had totally missedyew::suspense
also having a hook, that seems to be exactly what I was looking for…So now I'm wondering, what value do you see in
use_async
, that would not be covered byuse_future
already? AFAICT it's possible to just match on theSuspensionResult
instead of?
-ing it out, so I'm not seeing much value right now, but maybe I'm missing something?(Also, looking at the implementation of it resulted in this PR, but that's unrelated)
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 can confirm that I was able to replace my personal use of
use_async
withuse_future_with
. Maybe the main thing to improve aboutuse_future
would be re-exporting it fromyew::functional
, so that it's in the prelude, and maybe to document it a bit more prominently?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.
The value would be that
use_async
requires knowledge ofSuspension
and this might not always be what the user intends to do. Note that the following code is not the equivalent of what I'd expectuse_async
to do:In fact, dropping the
Suspension
- instead of returning it from a component function -will subsequently not lead to a re-render when the future resolves!will re-render but I'm not sure if that is intentional design in hindsight.Suspension
anduse_future
want to say: this component is not ready to render yet, wait for a bit until I tell you to try again.use_async
should say: Resolve this future, but I know what to do while the data is not there yet, keep on rendering.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 is a bit surprising to me, too. Do these components perhaps re-render because of some other thing happening instead of the Suspension resolving?
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.
Hmm is it supposed to not resolve? Looking at these lines:
output.set
will trigger a rerender, because theuse_state
value changed, andSuspension::from_future
will poll the future to completion even if dropped, so it will indeed be called at some pointIs my new code relying on an unintended feature that could disappear in the next version? 😅 (Right now I don't care much because it's only example code, but I'm planning on writing a
crdb-yew
crate next that'll be basically implementing lots of custom async-using yew hooks, so I'd rather not make that mistake there 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.
Oh alright, the implementation actually guarantees that your component will re-render even if you ignore the suspension. I'm not sure it should from a semantic perspective but that's not in scope for this PR. In any case, the creation of a suspension and suspend handle is really a bit useless if you don't want to suspend, so still I think this hook is worth something by avoiding useless work :)