Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Introduce a new use_async hook, that allows for setting dependencies #3609

Closed
wants to merge 2 commits into from
Closed

Introduce a new use_async hook, that allows for setting dependencies #3609

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
db14002
Introduce a new use_async hook, that allows for setting dependencies
Ekleog Feb 20, 2024
fef8b55
review comments
Ekleog Feb 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 2 additions & 0 deletions packages/yew/src/functional/hooks/mod.rs
View file
Open in desktop
@@ -1,3 +1,4 @@
mod use_async;
mod use_callback;
mod use_context;
mod use_effect;
Expand All @@ -9,6 +10,7 @@ mod use_ref;
mod use_state;
mod use_transitive_state;

pub use use_async::*;
pub use use_callback::*;
pub use use_context::*;
pub use use_effect::*;
Expand Down
56 changes: 56 additions & 0 deletions packages/yew/src/functional/hooks/use_async.rs
View file
Open in desktop
@@ -0,0 +1,56 @@
use std::{fmt, future::Future};

use crate::{functional::hook, use_effect_with, use_state, UseStateHandle};

#[hook]
Copy link
Member

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.

Copy link
Author

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)

Copy link
Author

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?

Copy link
Member

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.

Copy link
Member

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?

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?

Copy link
Author

@Ekleog Ekleog Feb 22, 2024 •
edited

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:

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)

Copy link
Member

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 :)

pub fn use_async<Arg, F, Fut, Res>(deps: Arg, f: F) -> UseAsyncHandle<Res>
Copy link
Member

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.

where
Arg: 'static + PartialEq,
F: 'static + FnOnce(&Arg) -> Fut,
Copy link
Member

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.

Fut: 'static + Future<Output = Res>,
Res: 'static,
{
let result = use_state(|| None);
use_effect_with(deps, {
Copy link
Member

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?

Copy link
Author

@Ekleog Ekleog Feb 21, 2024 •
edited

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?

Copy link
Member

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 result = result.clone();
move |deps| {
result.set(None);
let future = f(deps);
yew::platform::spawn_local(async move {
let res = future.await;
result.set(Some(res));
Copy link
Member

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 future f(deps1)
  • render with deps2, spawn future f(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 some result2 and sets this
  • f(deps1) resolves to some result1. I would consider it unintended to then present result1 to the user. The last time the component rendered, f(deps2) was requested, which shouldn't be overwritten.

});
}
});
UseAsyncHandle { inner: result }
}

#[derive(Clone, PartialEq)]
pub struct UseAsyncHandle<T> {
inner: UseStateHandle<Option<T>>,
}

#[derive(Debug)]
pub enum UseAsyncStatus<'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) -> UseAsyncStatus<'_, T> {
match &*self.inner {
None => UseAsyncStatus::Pending,
Some(res) => UseAsyncStatus::Ready(res),
}
}
}