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.

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Philipp-M
Copy link
Contributor

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 Reacts Suspense. 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 the Class view for both p and h1).

@casey
Copy link
Contributor

casey commented Jul 29, 2024

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 await_once, and it might be nice if memoized_await were renamed await_memoized to make their similarity slightly more obvious.

@casey
Copy link
Contributor

casey commented Jul 29, 2024

One thought, is it necessary to give await_once access to the initial state?

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

@Philipp-M
Copy link
Contributor Author

I like the name await_once, and it might be nice if memoized_await were renamed await_memoized to make their similarity slightly more obvious.

Yeah I think that makes sense.

Could it be:

I'm not sure, as the app_logic is evaluated quite often, so that async_function(state) is run multiple times, and may lead to weird/unexpected behavior, when it changes the state.

@Philipp-M
Copy link
Contributor Author

Regarding the state as initial argument, see this zulip topic for more thoughts, or in other words I'm not yet sure, but it may be interesting for the user to track some state, when this is evaluated first. Though the whole async story is still quite in flux I think.

@DJMcNab
Copy link
Member

DJMcNab commented Jul 29, 2024

I think the version directly accepting a future makes sense. It would need to move the fields it needs from state, but that's fine.

@Philipp-M
Copy link
Contributor Author

Philipp-M commented Jul 29, 2024

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 View::build.

@casey
Copy link
Contributor

casey commented Jul 30, 2024

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.

@flosse flosse added the web label Aug 12, 2024
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>) });
Copy link
Contributor

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?

Copy link
Contributor Author

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() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?
Copy link
Contributor

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 😉

Copy link
Contributor Author

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,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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.
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@flosse flosse left a 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.

@flosse
Copy link
Contributor

flosse commented Aug 15, 2024

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 View::build.

I have to admit, I'm still a bit confused about the Xilem approach, the feeling that everything in the world is a view.
But maybe I still don't understand the approach.
I mean, the probability that I have a large number of asynchronous tasks that don't have anything directly to do with the GUI at first, but actually only the effects of it, is very high, isn't it?
Here you would be tempted to pack all asynchronous tasks into views, which is not really architecturally clean, is it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants