yew icon indicating copy to clipboard operation
yew copied to clipboard

ChildrenWithProps<> doesn't implement iter_mut()

Open rakshith-ravi opened this issue 3 years ago • 8 comments

Problem

props.children doesn't implement iter_mut(). This makes it very difficult to iterate over the children and make a change on all of them (say, for example, set a value in the child props to false).

Steps To Reproduce Steps to reproduce the behavior:

pub struct GroupProps {
    pub children: ChildrenWithProps<Item>,
}

impl Component for Item {
    fn update(&mut self, msg: Self::Message) -> bool {
        self.props.children.iter_mut().for_each(|child| {
            child.is_selected = false;
        });
    }
}

Expected behavior props.children should implement iter_mut(), allowing us to iterate over the children and mutate them.

Screenshots None

Environment:

  • Yew version: v0.17
  • Rust version: 1.51.0
  • Target, if relevant: wasm32-unknown-unknown.
  • Build tool, if relevant: -
  • OS, if relevant: -
  • Browser and version, if relevant: -

Questionnaire

  • [x] I'm interested in fixing this myself but don't know where to start
  • [ ] I would like to fix and I have a solution
  • [ ] I don't have time to fix this right now, but maybe later

rakshith-ravi avatar Apr 07 '21 16:04 rakshith-ravi

You're probably using ChildrenWithProps::<_>::iter(); wrong. It doesn't allow mutation because you're supposed to mutate the elements, then render them to the VDom using Component::view();. See the example on the docs page

#[derive(Clone, Properties)]
struct ListProps {
    children: ChildrenWithProps<ListItem>,
}

impl Component for List {
    // ...
    fn view(&self) -> Html {
        html!{{
            for self.props.children.iter().map(|mut item| {
                item.props.value = format!("item-{}", item.props.value);
                item
            })
        }}
    }
}

I guess making that a feature of ChildrenWithProps::<_> is what you're asking, so that you can do mutations in other component functions, but I'm not sure if using it as state is a good idea. Just adds another opening for a bug to creep in.

frostu8 avatar Apr 08 '21 12:04 frostu8

I see your point of view, but what I'm trying to do is to make a RadioGroup component which hosts inner RadioButton components. Now what I want to do is to make a private on_change_listener inside the RadioButton which notifies the parent RadioGroup that a radio button has been clicked, so that it can disable the rest of the buttons in the same group. So when I detect a change event or in the create event, I'm assigning the on_change_listener to the parent to set the change listener. So this requires me to iterate over the (mutable) children as assign the listener to each of them. I'm open to suggestions on how to go about this.

rakshith-ravi avatar Apr 08 '21 13:04 rakshith-ravi

I actually made something like what you want to do yesterday. Here it is

The solution was to simply not use children and use a map. I wrote my own map that statically maps enums to an array, which has insanely low overhead, but using something like a HashMap would still have less overhead, since Yew does a lot of work for your component to participate in the DOM.

frostu8 avatar Apr 08 '21 14:04 frostu8

So you're saying we should create a new component by mapping the existing one in the render call? Isn't that expensive?

rakshith-ravi avatar Apr 09 '21 05:04 rakshith-ravi

So you're saying we should create a new component by mapping the existing one in the render call? Isn't that expensive?

I'm saying use a list of states rather than a list of components, and then render those states to the DOM.

frostu8 avatar Apr 09 '21 05:04 frostu8

So take a prop of a bunch of RadioButtons and render the buttons accordingly? I was thinking it'll make sense to have a RadioButton so that it can be used separately if we have to, and have it as the children of the RadioGroup to make it work together.

Also, apparently ChildrenRenderer<T> is implemented as a Vec<T> internally, so it should be pretty simple to implement iter_mut by simply calling the function on the Vec

rakshith-ravi avatar Apr 09 '21 05:04 rakshith-ravi

So take a prop of a bunch of RadioButtons and render the buttons accordingly?

I guess so. In Yew, I'd probably instead mark the indexes for each RadioButton component, something along the lines of this:

pub struct RadioGroup {
    children: ChildrenWithProps<RadioButton>,
    selected: usize,
}

impl Component for RadioGroup {
    // ...
    fn update(&mut self, msg: Msg) -> ShouldRender {
        match msg {
            // ...
            Msg::Update(selected) => {
                self.selected = selected;
            }
        }
    }

    fn view(&self) -> Html {
        html! {
            <div>{
                // now we assign an index to each radio button:
                for self.children.iter().enumerate().map(|(i, radio)| {
                    radio.props.onselect = self.link.callback(|_| Msg::Update(i));
                    radio.props.selected = self.selected == i;
                    radio
                })
            }</div>
        }
    }
}

frostu8 avatar Apr 09 '21 05:04 frostu8

Hi 👋

The state of things in Yew has changed since this issue was first raised. Properties are now stored in Rcs so are effectively readonly, so this is no longer as easy to implement. #1961 is the PR that introduced this change. In order to implement this solution now we'd need to introduce interior mutability for Context<Self> (bit like ComponentLink<Self> but holds Properties too and lives outside of the component state).

Let me know if you still want to request this feature or if the approach provided by @frostu8 (thank you btw ❤️) solved the need for the feature.

Housekeeping:

I guess making that a feature of ChildrenWithProps::<_> is what you're asking

Agreed, I have swapped the bug label for the feature request label :)

mc1098 avatar Sep 22 '21 09:09 mc1098