Material-ui: Concern over onChange of several components

[davidrenne]

I have been using material UI for over 2 years now and I have come to realize a possible design flaw to be discussed. Many of our forms are inline inside of one main render class with many input boxes and other complicated components and sometimes with components, things sometimes can render slow because our main form component has to re-render on each keypress from a user. This is because our onChange event is a typically a setState to set the state of the bound value to pass as props to each component we bind to.

• [x] This is a v1.x issue (v0.x is no longer maintained).
• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

I wanted to discuss some kind of possible change to allow the values to change inside of the component rather than making the caller pass value as a prop. By making the caller setState and set value into the parent state and passing it along as props, you have to re-render all sibling components who might hold state for other nodes just to get the value to be set according to what someone types.

Here is an example component which takes the value within the component rather than always relying on props passed to set the value, where its appropriate in this case to just set the value of the parent onBlur where a render could occur after all has been typed.

class Input extends BaseComponent {
  constructor(props, context) {
    super(props, context);
    this.state = {
      value: props.value,
      style: props.style
    };
  }
  render() {
    return <input style={this.state.style} key="input-label2" maxLength={this.props.maxLength} type='text' className='form-control' value={this.state.value}
    onChange={(e) => {
      this.setState({value: e.target.value});
    }}
    onBlur={() => {
      this.props.onBlur(this.state.value);
    }}/>
  }
}

I mostly just wanted opinions on this topic and maybe I can send a pull request of (at least for input boxes) allowing a version which sets the state within the component internally so that the parent setState doesnt re-render the entire (slow form if there are a lot of nodes).

My opinion is that its not an ideal practice to only rely on props to setState and not keep the state of values internal to your component. I have formed this opinion after years of seeing a slow typing response where my entire forms get progressively slower because material UI only relies on props passed to set the value. Each developer has to abstract your input boxes to their own component setting state to get it to work more efficiently to not render the entire "sibling form component"

Thank you for your time and thoughts on this matter. Maybe I am just doing something wrong and I am open to feedback of something I might be doing wrong.

Cheers!

[mbrookes]

We have intentionally been moving away from stateful components where possible.

Perhaps adding shouldComponentUpdate with a test for a change to the value prop would be a worthwhile tradeoff for form components. It would mean moving some functional components back to Classes, but they could remain stateless.

[oliviertassinari]

I have formed this opinion after years of seeing a slow typing response where my entire forms get progressively slower because material UI only relies on props passed to set the value.

@davidrenne This problem highlights the limitation of the brute-force approach. Updating the whole form component at each keystroke doesn't scale. But you are not the first one to face it. People have been building abstractions since the beginning of React to address it. I would encourage you to pick one of the popular React form library (react-final-form, react-redux, formik). You won't have this problem. They provide abstractions to keep each form field controlled and performant by scoping the updates.

I wanted to discuss some kind of possible change to allow the values to change inside of the component rather than making the caller pass value as a prop.

It's not something we will fix at the Material-UI level. As @mbrookes stated, we have designed the current situation on purpose, we are well aware of the tradeoff and happy with it.

[davidrenne]

That's cool guys. I'll provide this discussion to my team at least. Thank you for the great library guys!

[davidrenne]

I think if I just make a wrapper class for each component that handles the value changes in state in my own library then I could speed up any slow forms that must re-render the callers to this project (or at least just the TextField component)

Definitely there is a tradeoff for sure and its good sticking with your guns in the design, but I just wanted to voice an issue we see in directly importing them for building larger forms. Sure it's largely at each developers discretion as to what is good performance or not if a form is rendering slowly, but I at least wanted to discuss the possibility of making the TextField component more stateful because when an end user types and a parent form re-renders this is where we see the slowest response time because people are used to seeing what they type immediately, whereas this component could improve overall user experience and developer happiness if they didnt have to change the state on a parent object causing the entire form to re-draw rather than the individual text box.

If you think about all the components in this library, most of them are click oriented so the users can expect a single click or so and something to happen once the renders are done and if it takes 5 seconds to redraw, then that is sometimes acceptable. But for the TextField I think I will just provide an onBlur abstraction to setState on the parent when someone is done typing to set the value of state for later saving to the database so that each keystroke is not frustrating to users and makes us look like our app is sometimes slow depending on the processing power of the computer running the browser typing in this box.

[davidrenne]

For anyone in the same boat as our team where sometimes forms render slow/sluggish for TextField component and you come across this thread, here is an example abstraction which is stateful and isolated to just rendering that node and not all the nodes in your form.

https://gist.github.com/davidrenne/28e123e27417f3326aee18363f69bc77