Pauan / rust-dominator

Zero-cost ultra-high-performance declarative DOM library using FRP signals for Rust!
MIT License
967 stars 62 forks source link

Idiomatically implementing controlled components #7

Open aidanhs opened 5 years ago

aidanhs commented 5 years ago

See DenisKolodin/yew#233 (comment) for context.

Essentially, the desired behavior is: if an edit for a field (e.g. an input or text area) is 'rejected' (i.e. not applied to the model), the render should not update. In react this is known as 'controlled components'.

In dominator you can test this by applying the patch I've pasted at the bottom and then editing the 'new item' input element on the todomvc example. Because the internal state isn't changing, arguably neither should the render. Unfortunately, it does (due to default behaviour of the element).

React solves this by restoring state after a change event (described on the linked issue). Note that react also provides uncontrolled components where the value field is not set.

It is relatively easy to work around this in dominator (as opposed to diffing frameworks, where it's a little trickier) - just make sure to always do .set (i.e. not set_neq, which the current todomvc does) for any elements with relevant input events (I believe textarea, input, select as those are the ones react implements restoreControlledState for). This mimics the react behaviour of always resetting the controlled state of the element on an input event.

However, this seems subideal since you have to remember to do this for every element. I propose it should be straightforward to decide whether a component is controlled or uncontrolled - with react you decide by whether the value field is assigned. I also have a personal preference for controlled components but that's more subjective.

It is a valid response to say that dominator is a lower level library that shouldn't think about the semantics of different elements, but this does seem like a footgun when you end up wanting to control field inputs.

diff --git a/examples/todomvc/src/main.rs b/examples/todomvc/src/main.rs
index 3919949..3a5d84b 100644
--- a/examples/todomvc/src/main.rs
+++ b/examples/todomvc/src/main.rs
@@ -161,7 +161,7 @@ fn main() {
                             .property_signal("value", state.new_todo_title.signal_cloned())

                             .event(clone!(state => move |event: InputEvent| {
-                                state.new_todo_title.set_neq(get_value(&event));
+                                //state.new_todo_title.set_neq(get_value(&event));
                             }))

                             .event(clone!(state => move |event: KeyDownEvent| {
Pauan commented 5 years ago

I agree that "controlled components" are a good idea (the DOM should always match the app's state).

However, I don't like magic. So I wouldn't want to do something like "if there is a value set, then automagically make it a controlled component".

How does React handle controlled components? Does it just always define an onchange handler (which handles the value synchronization), or does it do something else?

Pauan commented 5 years ago

Oops, I should have read the other thread more carefully, you already covered that there.

Pauan commented 5 years ago

So, obviously we can't use the same strategy as React, since we don't do any DOM diffing.

I'll play around with creating a mixin that can solve this problem.

Pauan commented 5 years ago

After a few false starts, it was actually quite easy to create a mixin:

#[inline]
fn synchronized_event<A, B, E, F>(mutable: Mutable<A>, mut f: F) -> impl FnOnce(DomBuilder<B>) -> DomBuilder<B>
    where A: 'static,
          B: IEventTarget + Clone + 'static,
          E: ConcreteEvent,
          F: FnMut(E) + 'static {
    #[inline]
    move |dom| dom
        .event(move |e| {
            {
                // This triggers an update
                let _: &mut A = &mut mutable.lock_mut();
            }
            f(e);
        })
}

You use it like this:

html!("input", {
    .apply(synchronized_event(state.new_todo_title.clone(),
        clone!(state => move |event: InputEvent| {
            state.new_todo_title.set_neq(get_value(&event));
        })))
})

Now it will re-apply state.new_todo_title whenever the input event happens.

You can compare this to the non-synchronized event:

html!("input", {
    .event(clone!(state => move |event: InputEvent| {
        state.new_todo_title.set_neq(get_value(&event));
    }))
})

I've tried many different approaches, and the above seems the best so far. I don't really like it though, it seems awfully clumsy:

Is it even worth it to have synchronized components? In a typical implementation (such as the above, which simply sets new_todo_title to the value of the <input>), there should be no difference between synchronized and non-synchronized components, right?

Pauan commented 5 years ago

Also, it seems like there is a beforeinput event, which can be cancelled!

In that case it's really easy to create synchronized components:

html!("input", {
    .event(|e: BeforeInputEvent| {
        e.prevent_default();
    })
})

Even easier if the above is wrapped in a mixin:

html!("input", {
    .apply(synchronized())
})

However, beforeinput is still new, so it's not supported cross-browser.

Pauan commented 5 years ago

Here's a different approach:

#[inline]
fn value<A, B, C, F>(signal: A, mut f: F) -> impl FnOnce(DomBuilder<C>) -> DomBuilder<C>
    where A: Signal + 'static,
          B: JsSerialize,
          C: IEventTarget + Clone + 'static,
          F: FnMut(&A::Item) -> B + 'static {
    let (sender, receiver) = futures_signals::signal::channel(());

    #[inline]
    move |dom| dom
        .property_signal("value", map_ref! {
            let signal = signal,
            let _ = receiver => move {
                f(signal)
            }
        })
        .event(move |_: InputEvent| {
            sender.send(()).unwrap();
        })
}

You use it like this:

html!("input", {
    .event(clone!(state => move |event: InputEvent| {
        state.new_todo_title.set_neq(get_value(&event));
    }))

    .apply(value(state.new_todo_title.signal_cloned(), |a| a.clone()))
})

This seems like the most convenient so far, but it still doesn't feel quite right.

rsaccon commented 5 years ago

Generally I prefer the EventDispatcher approach, but this is less verbose and might be useful for certain use cases, e.g. re-implement a React component or so

Pauan commented 5 years ago

@rsaccon What do you mean by "the EventDispatcher approach"?

rsaccon commented 5 years ago

As discussed here: https://github.com/Pauan/rust-dominator/issues/6#issuecomment-454343134

Pauan commented 5 years ago

@rsaccon Oh, I see, that's a completely different issue, though. That issue is about how to make Rust components (and especially how those components communicate with their parent components).

This issue is about how to guarantee that the value of an <input> always matches the Mutable (even if the user is typing in the <input>).

David-OConnor commented 5 years ago

I don't suspect this is the most elegant approach, but I've added event listeners to (non-checkbox) input, select, and textarea elements that don't have input listeners, where if there's an input event, revert to previous value.

Haven't yet tackled keeping checkboxes in sync.

Pauan commented 5 years ago

@David-OConnor Yeah, that's similar to one of the solutions I posted.

It needs some thinking on how to avoid the |a| a.clone() though.