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 EventHandler trait and an additional defer event handler, shown in a new fetch example #440

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

Conversation

Philipp-M
Copy link
Contributor

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

Philipp-M commented Jul 20, 2024

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

Event: 'static,
FOut: Message,
F: Future<Output = FOut> + 'static,
FF: Fn(&mut State, Event) -> F + 'static,
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 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

... it would be like this, right?

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

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

@DJMcNab
Copy link
Member

DJMcNab commented Jul 22, 2024

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

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.

github-merge-queue bot pushed a commit that referenced this pull request Jul 26, 2024
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
Copy link
Contributor

casey commented Aug 10, 2024

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

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

I've moved the example to "fetch_event_handler"

@casey
Copy link
Contributor

casey commented Aug 11, 2024

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

Philipp-M commented Aug 11, 2024

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

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

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

casey commented Aug 11, 2024

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

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

Philipp-M commented Aug 11, 2024

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

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

@casey
Copy link
Contributor

casey commented Aug 11, 2024

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

casey commented Aug 11, 2024

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

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

casey commented Aug 11, 2024

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.

github-merge-queue bot pushed a commit that referenced this pull request Aug 12, 2024
…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 flosse added the web label Aug 12, 2024
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.

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

Niedzwiedzw commented Aug 27, 2024

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

flosse commented Aug 27, 2024

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 ☺️

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.

5 participants