-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
Size Comparison
✅ None of the examples has changed their size significantly. |
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 agree that having such a hook in yew
directly can be useful, especially considering the details the PR as-is gets wrong (see further comments) and indeed would most often be overlooked when reimplementing this in some third-party lib.
Thanks for the initiative!
Res: 'static, | ||
{ | ||
let result = use_state(|| None); | ||
use_effect_with(deps, { |
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.
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 comment
The 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 use_async
being useful with SSR? And if yes, is there any other hook I could use to define dependencies?
EDIT: NVM, I just saw use_memo
as an alternative to use_effect_with
. But I'm still not sure about what SSR is about and how that'd tie in with use_async
in particular?
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 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 use_memo
and just spawns the future, but then continues to render. The solution could be to just not poll the future on the server at all.
Note, because you also mention it in the other comment about use_future
: Suspensions do work on SSR (see there for a small explanation on what suspensions are) and wait for the future to complete, i.e. the suspension to resolve before rendering the component into the stream.
let future = f(deps); | ||
wasm_bindgen_futures::spawn_local(async move { | ||
let res = future.await; | ||
result.set(Some(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.
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:
- render with
deps1
, spawn futuref(deps1)
- render with
deps2
, spawn futuref(deps2)
- this could potentially cancel earlier jobs, but I don't recall wasm_bindgen_futures offering to cancel a spawned task so let's ignore that for now f(deps2)
resolves to someresult2
and sets thisf(deps1)
resolves to someresult1
. I would consider it unintended to then presentresult1
to the user. The last time the component rendered,f(deps2)
was requested, which shouldn't be overwritten.
pub fn use_async<Arg, F, Fut, Res>(deps: Arg, f: F) -> UseAsyncHandle<Res> | ||
where | ||
Arg: 'static + PartialEq, | ||
F: 'static + FnOnce(&Arg) -> Fut, |
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'm not sure if F
should be run as part of an effect, or just inline. Certainly the resulting future should be spawned later, but getting the future should not be effectful (running the future to completion does that!).
If you agree, please rewrite, and remove the 'static
bound here.
|
||
use crate::{functional::hook, use_effect_with, use_state, UseStateHandle}; | ||
|
||
#[hook] |
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 about use_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 missed yew::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 by use_future
already? AFAICT it's possible to just match on the SuspensionResult
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
with use_future_with
. Maybe the main thing to improve about use_future
would be re-exporting it from yew::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 of Suspension
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 expect use_async
to do:
let future: impl Future<Result = T> = todo!();
let foo = use_future(future).ok(); // An Option<T>, sucess?!
// let foo = use_async(future); // With this PR (fixed signature without dependencies)
// These two lines are not interchangeable.
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
and use_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.
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?
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:
- this
output.set
will trigger a rerender, because theuse_state
value changed, and Suspension::from_future
will poll the future to completion even if dropped, so it will indeed be called at some point
Is 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 :)
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 comment
The reason will be displayed to describe this comment to others. Learn more.
The convention with dependencies is to have two hooks, use_*_with
and use_*
one version with dependencies, the other if you don't need to have them. Please rename and implement the simpler one for consistency.
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.
Can you describe how this differs from use_future?
@hamza1311 you might not have seen my earlier comment |
Yep, totally missed that, my bad |
Considering the time I've been having recently, I think I unfortunately won't have any to come back to this PR. Going to close this, but anyone please feel free to take it over! :) |
Description
Introduce an
use_async
hook, that performs a computation in another task and returns the result.(Probably) fixes #364
The main API design question is whether we want to introduce the idea of cancellation in the API or not. Adding it afterwards would likely require adding a variant to
UseAsyncStatus
. This being said, Yew is not stable yet, and probably at least 90% of the use cases are covered by this API, so I think it makes sense to postpone the decision after verifying that there is an actual need.This PR is not done yet, and is here mostly to gather feedback on whether this is something that Yew developers would be interested in having upstream. I'm using it in my own code here, and thought it would likely be helpful to other people too.
The alternatives I know of are:
use_async
anduse_async_with_options
fromyew_hooks
. They do not support introducing dependencies that would trigger a future re-computation.yewtil
'sLinkFuture
. Not having used it I'm less familiar with it, but AFAIU it requires struct components, which is not the direction Yew development seems to be moving towards.Checklist
Things still missing if you confirm that this PR is of interest to you: