xilem_web: Add `EventHandler` trait and an additional `defer` event handler, shown in a new `fetch` example #440

Philipp-M · 2024-07-20T17:37:12Z

Adds an EventHandler trait, which can be used everywhere, where an event is expected to be invoked, like button("click me").on_click(impl EventHandler).

This trait is implemented by T: Fn(&mut State, Event) -> Action and a new DeferEventHandler which takes a callback returning a future, which after being resolved is given to another callback to modify the app state based on the output of the future.

The new DeferEventHandler is demonstrated in a new fetch example, which is a modified version of #427 (and thus also an alternative to that PR).

The EventHandler trait may be moved to xilem_core as I think it's generic enough to be able to be used in different contexts as well.
Though since it uses a blanket impl currently for F: Fn(&mut State, Event) -> Action + 'static, it would be necessary to write button("label", |data: &mut State, ()| {}) when we want to use it in the button, as we can't easily support F: Fn(&mut State) -> Action + 'static where Event = (), since those impls intersect. That may be solved by hardcoding event types for the impls, though I'm not sure if that's a good solution (and the orphan rule would make this even more complex).
This is less an issue in xilem_web since all event functions have a payload, so there's no Event = () currently.

It may or not be possible to have EventHandler<Event>: View, as the EventHandler is basically a view, without element and additional generic parameter Event. I have decided against this for now, because I think this just means suffering from the orphan rule. I may investigate this again, when it makes sense to integrate this with xilem_core, maybe there are ways, and maybe it really makes sense (in xilem_web this is currently not possible, because of the orphan rule).
I mostly fear impl<State, Action, Context, F> View<State, Action, Context, EventHandlerMessage> for F where F: Fn(&mut State, Event) -> Action, but maybe it is not as big of an issue as I fear.

Drawbacks:

Rust is unfortunately not able to infer the callback of such event handler callback functions.
So instead of el.on_click(|_, _| {}) it's now necessary to write el.on_click(|_: &mut _, _| {}).
I think it's not able to infer the lifetime of that first parameter.
I wonder, if there are ways to help rustc infer this without explicit (lifetime) type.

…tHandler`

…defer handler, with requiring an action type for the callback

Co-authored-by: Markus Kohlhase <[email protected]>

Philipp-M · 2024-07-20T19:02:43Z

After a short chat with Daniel, I think I get what he meant with declarative, so taking the fetch example:

fork(
    input(()).on_input(|state: &mut AppState, ev: web_sys::Event| {
        let count = event_target_value(&ev).parse::<usize>().unwrap_or(0);
        state.cats_to_fetch = count;
    }),
    (state.cats_to_fetch > 0).then_some(fetch(
        format!(
            "https://api.thecatapi.com/v1/images/search?limit={}",
            state.cats_to_fetch
        ),
       |state, fetch_result| { state.cats = do_something_with_result(fetch_result); }
    )),
),

this could be an alternative, and I see appeal in it (e.g. for the above, when the url changes, the current request will be canceled, and a new one started, and when state.cats_to_fetch == 0 it won't be inserted in the view tree at all, i.e. the fetch would not be run.

Though I think this has some implications, the above (fetch as view) is already quite opinionated, there couldn't be easily a defer view or run_once view accessing the app state (without weird/unexpected behavior).

I think such behavior could be achieved similarly as the Memoize view, so something like:

fork(
    input(()).on_input(|state: &mut AppState, ev: web_sys::Event| {
        let count = event_target_value(&ev).parse::<usize>().unwrap_or(0);
        state.cats_to_fetch = count;
    }),
    (state.cats_to_fetch > 0).then_some(rerun_on_change(
        state.cats_to_fetch,
        |count| fetch_cats(count),
        |state, output| {
            state.cats = do_something_with_result(output);
        },
    )),
),

I'm not sure, I see value for both approaches (the one of this PR, see also this comment, and the ideas in this comment (more declarative)).
I guess we need more data (user experiences/thoughts and real-world apps/use-cases).

Philipp-M · 2024-07-22T09:12:38Z

xilem_web/src/event_handler.rs

+    Event: 'static,
+    FOut: Message,
+    F: Future<Output = FOut> + 'static,
+    FF: Fn(&mut State, Event) -> F + 'static,


I think this could also be

Suggested change

FF: Fn(&mut State, Event) -> F + 'static,

FF: Fn(&mut State, Event) -> Option<F> + 'static,

which would give the user more control, if they want to handle the event or not, thus maybe make the blocking/once stuff above a non issue, as the user could handle this with more control.

I think the parallel/serial(queue) thing explained in the comment above may still be useful though for configuration.

But why would you call defer if you do not want to return a Future?

... it would be like this, right?

|state, ev|{ if state.value == some_value { None } else { Some(async { /* */ }) } }

DJMcNab · 2024-07-22T10:49:02Z

Yes, rerun_on_change (or whatever the name becomes would match my intention).

I do think there is value in being able to launch ad-hoc tasks, but I'd like to push as far on the declarative approach as possible, because I do think it has some real advantages.

I'm not sure whether I'm going to review this PR.

Philipp-M · 2024-07-22T12:09:49Z

Yes, rerun_on_change (or whatever the name becomes would match my intention).

I do think there is value in being able to launch ad-hoc tasks, but I'd like to push as far on the declarative approach as possible, because I do think it has some real advantages.

Yeah, I think I'm going to check this as well, as I see value for it either way, including specialized views, like the fetch above it.
This PR is not optimal mostly because of the rust inference issues (Drawbacks in the description), which I would like to avoid, because I imagine users running into this and the errors I got remind me a bit on C++ template errors (we should investigate error messaging and optimization of these non-the-less, so maybe with a better error, this becomes less of an issue). The inference issue also persists with the new trait-solver.

This is the `rerun_on_change` view described in #440, I named it `MemoizedAwait` as I think that fits its functionality. It got also additional features `debounce_ms` (default `0`) and `reset_debounce_on_update` (default `true`) as I think that's quite useful, in case a lot of updates are happening. When `reset_debounce_on_update == false`, `debounce` is more a throttle than an actual debounce. This also adds a more heavily modified version of the example added in #427, also showing `OneOf` in xilem_web (which didn't have an example yet). This *could* be considered as alternative to #440 (at the very least the example) but those approaches could also well live next to each other. --------- Co-authored-by: Markus Kohlhase <[email protected]>

casey · 2024-08-10T17:33:25Z

Would it be possible to rebase/merge latest into this branch? I really want to try this PR out, but I'm dependent on a few things which were added since it was opened, and don't know how to fix the merge conflicts myself.

Philipp-M · 2024-08-11T10:22:05Z

Merged main into this, there happened quite a bit more than I thought...

I've moved the example to "fetch_event_handler"

casey · 2024-08-11T18:16:56Z

Nice!

I'm trying it out but getting an error. My code looks like this:

ul(li(button("Node").on_click(defer(
  |_: &mut State, _| async { Api::default().node().await.unwrap() },
  |state: &mut State, node| state.view = View::Node { node },
)))),

Which I think is basically just:

button("foo").on_click(defer(
  |_, _| async_bar(),
  |state: &mut State, results| state.results = results,
))

I'm getting this error in the console when I click the button:

loader.js:755 Uncaught Error: `unwrap_throw` failed (/Users/rodarmor/.cargo/git/checkouts/xilem-6e892874d75acc24/c0adafd/xilem_web/src/elements.rs:81:42)
    at imports.wbg.__wbindgen_throw (loader.js:755:15)
    at library_viewer-9afbf19c1f90bf56.wasm.wasm_bindgen::throw_str::h0f3d9e326f2c4e55 (index.wasm:0x28143f)
    at library_viewer-9afbf19c1f90bf56.wasm.<core::option::Option<T> as wasm_bindgen::UnwrapThrowExt<T>>::expect_throw::h824c473f1192e77a (index.wasm:0x23b068)
    at library_viewer-9afbf19c1f90bf56.wasm.wasm_bindgen::UnwrapThrowExt::unwrap_throw::hd67fe54312c8c148 (index.wasm:0x1296e7)
    at library_viewer-9afbf19c1f90bf56.wasm.<S as xilem_web::elements::DomViewSequence<State,Action>>::dyn_seq_rebuild::hb355c9ab0222379c (index.wasm:0x2178a1)
    at library_viewer-9afbf19c1f90bf56.wasm.xilem_web::elements::rebuild_element::h9ea9214dccfc32fc (index.wasm:0x1c176f)
    at library_viewer-9afbf19c1f90bf56.wasm.<xilem_web::elements::html::Div<State,Action> as xilem_core::view::View<State,Action,xilem_web::context::ViewCtx,alloc::boxed::Box<dyn xilem_web::message::Message>>>::rebuild::hca4c118f7ce878be (index.wasm:0x24b9fd)
    at library_viewer-9afbf19c1f90bf56.wasm.<V as xilem_core::any_view::AnyView<State,Action,Context,DynamicElement,Message>>::dyn_rebuild::{{closure}}::{{closure}}::h6f1e5b594af6c539 (index.wasm:0x1fc608)
    at library_viewer-9afbf19c1f90bf56.wasm.xilem_core::view::ViewPathTracker::with_id::h1261e98084ee8ea0 (index.wasm:0x278dd8)
    at library_viewer-9afbf19c1f90bf56.wasm.<V as xilem_core::any_view::AnyView<State,Action,Context,DynamicElement,Message>>::dyn_rebuild::{{closure}}::h2b0615db0bce8c21 (index.wasm:0x1e64e8)

All of which seems to be from within xilem, so I don't think my code is the culprit.

Without using this branch, I have a two-step process where I set an enum on the state to a variant that represents a pending request:

        ul(li(button("Node").on_click(|state: &mut State, _| {
          state.view = View::NodeQuery;
        }))),

And then in the update function:

    match state.view {
      View::NodeQuery => fork(
        div(()),
        memoized_await(
          (),
          |()| async { Api::default().node().await.unwrap() },
          |state: &mut State, node| state.view = View::Node { node },
        ),
      )
      .boxed(),
    }

Which is working.

Philipp-M · 2024-08-11T19:03:02Z

Is that with a debug build? I wonder if there could be a more detailed error-trace (type erasure of the ViewSequence makes things more difficult unfortunately though anyway). At a skim, it doesn't look like an issue with this PR, more like a bug triggered/surfaced by it.

The type of a DomFragment shouldn't be allowed to change by xilem_web, which looks like is the case here. Maybe an issue in the AnyView implementation?

Edit: Ah I have a feeling what it could be, and it isn't encouraging (or needs at least more fundamental changes in the internal DomViewSequence): The type itself doesn't change, as it is erased, but the underlying erased type is... Hmm that's bad, DomViewSequence is mostly an internal optimization for reducing compilation time...

Philipp-M · 2024-08-11T19:09:28Z

Or in other words: I think I need to rewrite #158 to be compatible with the new xilem_core to keep that compiler-time-optimization (and I hope we can remove that workaround with type-erasure when the new trait-solver is finished, as that would guarantee a fully static view again, but I doubt it when the app gets really big)

Philipp-M · 2024-08-11T19:17:28Z

In the meantime I would recommend using OneOf/Either instead of AnyView if possible to avoid that bug, as it should be caused by it (though as explained, I don't think the issue is in AnyView).

casey · 2024-08-11T19:34:52Z

I came up with a minimal reproduction, and you're right, it looks like it isn't actually related to this branch. Here's the repro:

fn update(state: &mut bool) -> impl DomFragment<bool> {
  use html::*;

  (
    button("foo").on_click(|state: &mut bool, _| *state = true),
    if *state { div(()) } else { div("bar") },
  )
}

#[wasm_bindgen(main)]
fn main() -> Result<(), JsValue> {
  App::new(
    web_sys::window()
      .unwrap()
      .document()
      .unwrap()
      .body()
      .unwrap(),
    false,
    update,
  )
  .run();
  Ok(())
}

Clicking the button gives the same error:

loader.js:464 Uncaught Error: `unwrap_throw` failed (/Users/rodarmor/.cargo/git/checkouts/xilem-6e892874d75acc24/c0adafd/xilem_web/src/elements.rs:81:42)
    at imports.wbg.__wbindgen_throw (loader.js:464:15)
    at library_viewer-9afbf19c1f90bf56.wasm.wasm_bindgen::throw_str::h0f3d9e326f2c4e55 (index.wasm:0x3e473)
    at library_viewer-9afbf19c1f90bf56.wasm.<core::option::Option<T> as wasm_bindgen::UnwrapThrowExt<T>>::expect_throw::hc4e86d62cd531f43 (index.wasm:0x34fa7)
    at library_viewer-9afbf19c1f90bf56.wasm.wasm_bindgen::UnwrapThrowExt::unwrap_throw::h51dbc0a749d849b7 (index.wasm:0x11ae2)
    at library_viewer-9afbf19c1f90bf56.wasm.<S as xilem_web::elements::DomViewSequence<State,Action>>::dyn_seq_rebuild::h83d448a68910e3c9 (index.wasm:0x30549)
    at library_viewer-9afbf19c1f90bf56.wasm.xilem_web::elements::rebuild_element::h7ee62349d2fc2a22 (index.wasm:0x243dc)
    at library_viewer-9afbf19c1f90bf56.wasm.<xilem_web::elements::html::Div<State,Action> as xilem_core::view::View<State,Action,xilem_web::context::ViewCtx,alloc::boxed::Box<dyn xilem_web::message::Message>>>::rebuild::h63fe93556904299a (index.wasm:0x36eb1)
    at library_viewer-9afbf19c1f90bf56.wasm.<V as xilem_core::sequence::ViewSequence<State,Action,Context,Element,Message>>::seq_rebuild::{{closure}}::{{closure}}::h9ede62a0c7b7457c (index.wasm:0x39fc4)
    at library_viewer-9afbf19c1f90bf56.wasm.<xilem_web::Pod<xilem_web::DynNode,alloc::boxed::Box<dyn core::any::Any>> as xilem_core::element::SuperElement<xilem_web::Pod<E,P>>>::with_downcast_val::h37f145e3aba5c13e (index.wasm:0x26198)
    at library_viewer-9afbf19c1f90bf56.wasm.xilem_core::element::SuperElement::with_downcast::h2dda4e0bbd415ab2 (index.wasm:0x39418)

Should I open a separate issue for this?

xilem_web/web_examples/fetch_event_handler/src/main.rs

Philipp-M · 2024-08-11T19:42:39Z

Should I open a separate issue for this?

I thought about it already, yeah, ideally with a minimal repro. I think something like this may already be enough (not yet tested though):

fn component<State>(condition: bool) -> impl DomView<State> {
    if condition {
        div(()).boxed()
    } else {
        div("different child type").boxed()
    }
}

Philipp-M · 2024-08-11T19:52:49Z

with a minimal reproduction

>.< darn I haven't read that far, looks quite similar as my one, and confirms my suspicion.
I have one idea I hope that's sufficient to work around it (the AnyViewSequence is useful anyway though), by adding the ViewSequence as PhantomData to the elements, which I hope avoids the compile time issue, while providing unique types...

Philipp-M · 2024-08-11T20:24:59Z

@casey Can you check if #505 fixes the issue? (it does for me)

casey · 2024-08-11T20:36:28Z

@Philipp-M Yup, that fixed it! I checked out #505 locally and merged in #440 (since I'm trying outdefer), and it now works.

casey · 2024-08-11T21:42:38Z

And, to be clear, defer() also works great, it lets me avoid a lot of intermediate state which is just there to track that I'm waiting for an async response.

Philipp-M · 2024-08-11T23:00:15Z

it lets me avoid a lot of intermediate state which is just there to track that I'm waiting for an async response.

That's also my thinking with it, I do think the declarative way (i.e. memoized_await, await_once) has appeal though, as it's obvious that there's only one future running, and likely leads to a more defined app-state.
But on the other hand, I think quite often one wants to just start a future on an event, and either don't care if there are other futures running, or as stated in the comments in the defer view impl, could be queued/blocked etc. by a modifier to that handler. Hmm I'm not sure.
I would lean to merge, if there wouldn't be the inference issues + slightly more complex on_event signature, which I don't like...

casey · 2024-08-11T23:06:37Z

it lets me avoid a lot of intermediate state which is just there to track that I'm waiting for an async response.

That's also my thinking with it, I do think the declarative way (i.e. memoized_await, await_once) has appeal though, as it's obvious that there's only one future running, and likely leads to a more defined app-state.

I definitely agree, and in fact, I think it's possible that I might wind up using both memoized_await / await_once and defer in different places. For example, if you click a button, dispatch a future, and then display the results of that future to the user, you might want some kind of intermediate display where you indicate that you're waiting for a response, which is easy to do if you set the app state to indicate that you're waiting for a future on click, and then when you update display the intermediate pending state, and then when you get a response from the future finally display the result.

…om `AnyView` (#505) This should fix the issue described in #440 by adding the type of the `Children` `ViewSequence` to the type of element, which results in unique types, so `AnyView` should not cause issues anymore. Fortunately there's no regression with the compilation times here, so an `AnyViewSequence` doesn't seem to be necessary in that case.

flosse

To be honest, I can't really keep track of the PR right now.
Maybe I'll have time to read through it again at the weekend.
But before it comes to a blockade, please talk to me directly about it again, maybe I can help specifically.

Niedzwiedzw · 2024-08-27T08:26:58Z

I'm not a xilem developer - just a user - so not sure if it's ok for me to write here, but I like this API and I'd really like to see this merged :)

flosse · 2024-08-27T16:04:57Z

I'm not a xilem developer - just a user - so not sure if it's ok for me to write here, but I like this API and I'd really like to see this merged :)

I think @Philipp-M is busy with other things at the moment (like me), but I would definitely be willing to help ☺️

Philipp-M and others added 5 commits July 20, 2024 16:06

xilem_web: Abstract callback of OnEvent-like views away with an `Even…

b20b788

…tHandler`

Add a defer EventHandler to xilem_web

2081652

xilem_web: Make interfaces generic over EventHandler and 'fix' the …

178e2b9

…defer handler, with requiring an action type for the callback

xilem_web: Add fetch example (modified version of linebender#427)

e7a8e3c

Co-authored-by: Markus Kohlhase <[email protected]>

Fix CI (tests, 'typo', license header)

8db04ab

Philipp-M mentioned this pull request Jul 20, 2024

xilem_web: Add fetch example #427

Closed

Philipp-M commented Jul 22, 2024

View reviewed changes

Philipp-M mentioned this pull request Jul 24, 2024

xilem_web: Add a MemoizedAwait view #448

Merged

Merge branch 'main' into xilem_web-event-handler

c0adafd

flosse reviewed Aug 11, 2024

View reviewed changes

xilem_web/web_examples/fetch_event_handler/src/main.rs Show resolved Hide resolved

Philipp-M mentioned this pull request Aug 11, 2024

xilem_web: Fix issue with downcasting to a wrong type transitively from AnyView #505

Merged

Philipp-M mentioned this pull request Aug 12, 2024

xilem_web: Add event_target_value helper function #506

Merged

flosse added the web label Aug 12, 2024

flosse reviewed Aug 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xilem_web: Add `EventHandler` trait and an additional `defer` event handler, shown in a new `fetch` example #440

xilem_web: Add `EventHandler` trait and an additional `defer` event handler, shown in a new `fetch` example #440

Philipp-M commented Jul 20, 2024

Philipp-M commented Jul 20, 2024 •

edited

Loading

Philipp-M Jul 22, 2024

flosse Jul 22, 2024

flosse Jul 22, 2024

Philipp-M Jul 22, 2024

DJMcNab commented Jul 22, 2024

Philipp-M commented Jul 22, 2024

casey commented Aug 10, 2024

Philipp-M commented Aug 11, 2024

casey commented Aug 11, 2024 •

edited

Loading

Philipp-M commented Aug 11, 2024 •

edited

Loading

Philipp-M commented Aug 11, 2024

Philipp-M commented Aug 11, 2024

casey commented Aug 11, 2024

Philipp-M commented Aug 11, 2024

Philipp-M commented Aug 11, 2024 •

edited

Loading

Philipp-M commented Aug 11, 2024

casey commented Aug 11, 2024

casey commented Aug 11, 2024

Philipp-M commented Aug 11, 2024

casey commented Aug 11, 2024 •

edited

Loading

flosse left a comment

Niedzwiedzw commented Aug 27, 2024 •

edited

Loading

flosse commented Aug 27, 2024

	FF: Fn(&mut State, Event) -> F + 'static,
	FF: Fn(&mut State, Event) -> Option<F> + 'static,

xilem_web: Add EventHandler trait and an additional defer event handler, shown in a new fetch example #440

Are you sure you want to change the base?

xilem_web: Add EventHandler trait and an additional defer event handler, shown in a new fetch example #440

Conversation

Philipp-M commented Jul 20, 2024

Philipp-M commented Jul 20, 2024 • edited Loading

Philipp-M Jul 22, 2024

Choose a reason for hiding this comment

flosse Jul 22, 2024

Choose a reason for hiding this comment

flosse Jul 22, 2024

Choose a reason for hiding this comment

Philipp-M Jul 22, 2024

Choose a reason for hiding this comment

DJMcNab commented Jul 22, 2024

Philipp-M commented Jul 22, 2024

casey commented Aug 10, 2024

Philipp-M commented Aug 11, 2024

casey commented Aug 11, 2024 • edited Loading

Philipp-M commented Aug 11, 2024 • edited Loading

Philipp-M commented Aug 11, 2024

Philipp-M commented Aug 11, 2024

casey commented Aug 11, 2024

Philipp-M commented Aug 11, 2024

Philipp-M commented Aug 11, 2024 • edited Loading

Philipp-M commented Aug 11, 2024

casey commented Aug 11, 2024

casey commented Aug 11, 2024

Philipp-M commented Aug 11, 2024

casey commented Aug 11, 2024 • edited Loading

flosse left a comment

Choose a reason for hiding this comment

Niedzwiedzw commented Aug 27, 2024 • edited Loading

flosse commented Aug 27, 2024

xilem_web: Add `EventHandler` trait and an additional `defer` event handler, shown in a new `fetch` example #440

xilem_web: Add `EventHandler` trait and an additional `defer` event handler, shown in a new `fetch` example #440

Philipp-M commented Jul 20, 2024 •

edited

Loading

casey commented Aug 11, 2024 •

edited

Loading

Philipp-M commented Aug 11, 2024 •

edited

Loading

Philipp-M commented Aug 11, 2024 •

edited

Loading

casey commented Aug 11, 2024 •

edited

Loading

Niedzwiedzw commented Aug 27, 2024 •

edited

Loading