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

Idiomatically implementing controlled components #7

Open
aidanhs opened this issue Jan 27, 2019 · 12 comments
Open

Idiomatically implementing controlled components #7

aidanhs opened this issue Jan 27, 2019 · 12 comments

Comments

@aidanhs
Copy link

aidanhs commented Jan 27, 2019

See yewstack/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
Copy link
Owner

Pauan commented Jan 30, 2019

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

Pauan commented Jan 30, 2019

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

@Pauan
Copy link
Owner

Pauan commented Jan 30, 2019

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

Pauan commented Feb 4, 2019

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:

  • The need to pass in a Mutable is not great

  • The fact that it only supports a single Mutable isn't great either

  • The fact that you have to use synchronized_event once for each event that you care about (e.g. change, input, etc.) isn't great

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

Pauan commented Feb 4, 2019

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

Pauan commented Feb 4, 2019

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

rsaccon commented Feb 4, 2019

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

Pauan commented Feb 6, 2019

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

@rsaccon
Copy link

rsaccon commented Feb 6, 2019

As discussed here: #6 (comment)

@Pauan
Copy link
Owner

Pauan commented Feb 9, 2019

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

David-OConnor commented Feb 10, 2019

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

Pauan commented Feb 14, 2019

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

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

No branches or pull requests

4 participants