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

[Philipp-M]

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.

[Philipp-M]

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

[DJMcNab]

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]

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.

[casey]

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]

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

I've moved the example to "fetch_event_handler"

[casey]

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]

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]

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]

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]

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?

[Philipp-M]

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]

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]

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

[casey]

@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]

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]

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]

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.

[Niedzwiedzw]

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]

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

[flosse]

@Philipp-M what's the current state here?

I tried to merge or rebase this branch but I ended up with manually picking the changes: https://github.com/slowtec/xilem/commit/c571e41a44159aaa1dc82113765b38a9237dc0bd

Is there a chance that we can can merge this soon? Is there anything I can do?

[Philipp-M]

I'm a little bit short on time currently... I'm fine with merging this (as feedback seems to be positive so far). But I'd be happy about feedback before from other maintainers to avoid diverging with xilem_web too much, in case this is not a direction to consider for Xilem native.