Material-ui: [Input] Should determine if the component is controlled at the mount time

[oliviertassinari]

• [x] I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior

The controlled/uncontrolled behavior is determined at the mount time.

Current Behavior

The controlled/uncontrolled behavior is determined at each render.

Steps to Reproduce (for bugs)

Look at the source code.

Context

@kgregory Raised this issue in #9523. It's important to notice that the Input component is the only component behaving this way. It's no the case for: input, Checkbox, Switch, Radio, ExpansionPanel and Tooltip.

Your Environment

| Tech | Version |
|--------------|---------|
| Material-UI | v1.0.0-beta.24 |
| React | v16.2.0 |

[kgregory]

Thanks @oliviertassinari!

cc @hivemind9000

[kgregory]

Thinking about this, we need to be careful with how we determine whether the component is controlled. We currently check value for undefined. I am pretty sure it isn’t out of the ordinary for people to map their forms to results from a server call which could include an undefined attribute (or exclude the attribute altogether). They may even map it to undefined while a request is being issued. If we move for making Input controlled at first mount, users have one shot at it, and the scenarios above would break.

The current code makes the component controlled whenever a value has been provided in props and I’m sure people are relying on that behavior. So caution is warranted.

[Hivemind9000]

Core React tends to frown upon switching from uncontrolled to controlled in components, but it seems that in real world applications this is almost unavoidable (for the reasons @kgregory has mentioned, and a number of others I have come across - particularly when the controlling data is coming from an external source).

The approach of implicitly determining the intention to control the component, based on an initial value, seems a little limited. As a developer, if I specifically set the prop in JSX I am signalling my intention to control the component. As it stands I'd have to do a null/undefined check for every controlled component I use (and I always want to control components).

If the goal of this framework is to be simple and intuitive to use, then I think that making components respond dynamically (post-instantiation) to changes in internal/external control is the better approach. Another option is to explicitly set the isControlled state as a separate component prop (probably defaulting to true - as that should generally be the prevailing use case) - leaving no ambiguity as for the intention of the developer.

As mentioned above, if you change Input (and now ExpansionPanel) to be consistent with the other components, a lot of developers' apps will break. I'd prefer the status quo rather than that happening.

[oliviertassinari]

The approach of implicitly determining the intention to control the component, based on an initial value, seems a little limited.

@Hivemind9000 The tradeoff is to let users handle this. One could argue that handling both are making things more complicated for our users. I never had this use case after years of playing with React. Isn't it creating an unneeded mental overhead? Is it simpler to pick one behavior and to stick it to it? Most of the time using both behaviors is creating tricky issues. React input used to work like our Input. The react team changed the behavior recently (1 year ago, I can find the release if you ask). I think that we can learn from them.

Yes, it would be a breaking change. I'm confident we can rely on React warnings to let our users spot the new behavior.

[Hivemind9000]

@oliviertassinari Fair enough, it's your call - I agree about not making it more complicated (it was more a thought experiment about explicit intention vs implicit behavior).

I dug around to try and understand why React have frowned upon controlled state switching. It appears to only relate to components wrapping the underlying DOM elements of <input> <textarea> and <select>, as they are either directly controlling these elements, or handing control over to the underlying engine. So I'm not convinced it's a convention - more so a pragmatic wariness (possibly due to performance and/or potential stability issues?) for those specific elements.

One thing to be aware of is that users are already get the warning from react, but it still seems to work (switch control). So I don't think they'll notice anything except their code suddenly not working. Perhaps a new warning in Input should be added? Might be nice to have similar warnings if you're not allowing control switching in other components (or just a note in the docs relating to those specific components/props could be equally effective).

[mikew]

Wouldn't isControlled = Object.hasOwnProperty.call(this.props, 'value') make the most sense here?

[oliviertassinari]

@mikew Your suggestion is less permissive. We follow React implementation:
React

function isControlled(props) {
  const usesChecked = props.type === 'checkbox' || props.type === 'radio';
  return usesChecked ? props.checked != null : props.value != null;
}

Input.js
https://github.com/mui-org/material-ui/blob/a585570267bd6d589172ce184d968511b0264206/src/Input/Input.js#L14-L16

[oliviertassinari]

Actually, let's use != null to save some bytes. We have the tests to cover our back.