-
Notifications
You must be signed in to change notification settings - Fork 120
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
xilem_web: Add a await_once
view, and a simple example suspense
showing it in action.
#452
base: main
Are you sure you want to change the base?
Conversation
…howing it in action.
Just tried this locally, and it seems like a nice addition! I was able to rewrite this: memoized_await(
(),
|()| async {
Api::default()
.packages()
.await
.unwrap()
.into_iter()
.collect::<Vec<(Hash, Manifest)>>()
},
|state: &mut State, output| state.packages = output,
),
memoized_await(
(),
|()| async { Api::default().handlers().await.unwrap() },
|state: &mut State, output| state.handlers = output,
), Into: await_once(
|_| async {
Api::default()
.packages()
.await
.unwrap()
.into_iter()
.collect::<Vec<(Hash, Manifest)>>()
},
|state: &mut State, output| state.packages = output,
),
await_once(
|_| async { Api::default().handlers().await.unwrap() },
|state: &mut State, output| state.handlers = output,
), I like the name |
One thought, is it necessary to give Instead of: await_once(
|state| async { async_function(state).await },
|state: &mut State, output| state.foo = output,
), Could it be: await_once(
async_function(state), // since we're probably calling await_once in a context where `state` is in scope
|state: &mut State, output| state.foo = output,
), And when constructing the future doesn't require access to the state at all: await_once(
async_function(),
|state: &mut State, output| state.foo = output,
), |
Yeah I think that makes sense.
I'm not sure, as the |
Regarding the |
I think the version directly accepting a future makes sense. It would need to |
I was more thinking about having an "event" when the future is actually initialized, but as said I'm not sure about it, so having a future there directly maybe makes sense as well. Hmm I'm not sure about especially this init future event, I think it could certainly be useful, to really know when a future has started, but as noticed in the fetch example, also leads to more imperative code, not sure how we should handle these kind of "events" in general... Another issue with having a future here directly is also, that it may lead to surprising behavior when the future itself changes, as it would not have any effect, as it would then be invoked in |
Actually, thinking more about it, I think requiring a function/closure which takes the current state and returns a future is perhaps best for clarity, i.e., reinforcing that the closure will only be called once, as opposed to taking a future each time, but only evaluating that future once. |
let thunk = ctx.message_thunk(); | ||
// we can't directly push the initial message, as this would be executed directly (not in the next microtick), which in turn means that the already mutably borrowed `App` would be borrowed again. | ||
// So we have to delay this with a spawn_local | ||
spawn_local(async move { thunk.push_message(None::<FOut>) }); |
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.
As far as I can remember, we already have this situation in several use cases. I wonder if it is time to either extend the MessageThunk
with e.g. fn send_message_asynchronously
, or if there is another common solution?
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.
Yes I've thought about this as well, makes sense. Maybe we want to rename push_message
to handle_message
(so that it's more clear, that it will be handled immediately, which is probably relevant for events) and add a new method enqueue_message
or defer_message
with this inside.
ctx: &mut ViewCtx, | ||
(): Mut<'el, Self::Element>, | ||
) -> Mut<'el, Self::Element> { | ||
if let Some(future) = view_state.future.take() { |
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.
if let Some(future) = view_state.future.take() { | |
let Some(future) = view_state.future.take() else { | |
return; | |
}; |
message: DynMessage, | ||
app_state: &mut State, | ||
) -> MessageResult<Action, DynMessage> { | ||
assert!(id_path.is_empty()); // `debug_assert!` instead? to save some bytes in the release binary? |
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.
..yes, I almost always use debug_assert
, but mostly in projects where the potential bugs should actually be noticed during development. I don't know enough about the internals of Xilem to judge the likelihood of this.
In any case, it is an advantage to create a panic as early as possible if something is obviously fundamentally wrong.
Since Xilem is still under development, an early bug report from users is probably also very desirable.
In other words: at the moment, it would be even more important to get early feedback on errors than to save bytes 😉
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.
Yes, my thinkings similar, it's probably better to get bug reports when this is somehow triggered.
where | ||
State: 'static, | ||
Action: 'static, | ||
FOut: std::fmt::Debug + '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.
FOut: std::fmt::Debug + 'static, | |
FOut: fmt::Debug + 'static, |
....and of course use std::fmt;
at the top. Not important, just a personal preference 😉
phantom: PhantomData<fn() -> (State, Action)>, | ||
} | ||
|
||
/// Await a future returned by `init_future`, `callback` is called with the output of the future. `init_future` will only be invoked once. |
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.
Maybe a sentence about the fact that a change of &mut AppState
has no effect at all and that if it should be so, then async_repeat
must be used for it?
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 mean a change in AppState
has effect (in a rerender)? It just won't have an effect when using interior mutability within e.g. the future. (Which we should probably generally document).
That was actually a reason why I wanted to include it in the init_future
so that some kind of is_loading
state could be set.
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, looks good so far, i just added a few low-priority thoughts.
I have to admit, I'm still a bit confused about the Xilem approach, the feeling that everything in the world is a view. |
This adds a view similar as
xilem_web::concurrent::memoized_await
with the difference, that it's not having any memoized data, and instead takes the app state as first parameter, the future is run just once.This is demonstrated within a new
suspense
example, which shows, that this view could be used similar as something like ReactsSuspense
. This doesn't necessarily mean, that there won't be a more specialized view for this use-case though.It also shows a slightly more interesting use-case of
Either
(accessing theClass
view for bothp
andh1
).