-
Notifications
You must be signed in to change notification settings - Fork 65
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
Comments
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 How does React handle controlled components? Does it just always define an |
Oops, I should have read the other thread more carefully, you already covered that there. |
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. |
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 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 |
Also, it seems like there is a 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, |
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. |
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 |
@rsaccon What do you mean by "the EventDispatcher approach"? |
As discussed here: #6 (comment) |
@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 |
I don't suspect this is the most elegant approach, but I've added event listeners to (non-checkbox) Haven't yet tackled keeping checkboxes in sync. |
@David-OConnor Yeah, that's similar to one of the solutions I posted. It needs some thinking on how to avoid the |
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. notset_neq
, which the current todomvc does) for any elements with relevant input events (I believe textarea, input, select as those are the ones react implementsrestoreControlledState
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.
The text was updated successfully, but these errors were encountered: