Warn when an input switches between controlled and uncontrolled #5864
Conversation
| @@ -141,6 +143,7 @@ var ReactDOMInput = { | |||
| inst._wrapperState = { | |||
| initialChecked: props.defaultChecked || false, | |||
| initialValue: defaultValue != null ? defaultValue : null, | |||
| controlled: props.value !== undefined, | |||
There was a problem hiding this comment.
Ideally this should be DEV only.
|
@sebmarkbage I've added the line specified to DEV only |
|
@TheBlasfem updated the pull request. |
| @@ -144,6 +146,10 @@ var ReactDOMInput = { | |||
| listeners: null, | |||
| onChange: _handleChange.bind(inst), | |||
| }; | |||
|
|
|||
| if (__DEV__) { | |||
| inst._wrapperState.controlled = props.value !== undefined; | |||
There was a problem hiding this comment.
If type="checkbox" then the controlled component would use the checked prop instead of the value prop.
|
thanks @jimfb I've sent a commit, i'm taking into account now the checkbox inputs |
|
@TheBlasfem updated the pull request. |
|
I'm not sure where we stand technically (do we still track owner?), but if possible the errors should include the responsible component (or source location) and be tracked individually per component? These generic warnings are incredibly hard to diagnose. |
| @@ -144,6 +146,11 @@ var ReactDOMInput = { | |||
| listeners: null, | |||
| onChange: _handleChange.bind(inst), | |||
| }; | |||
|
|
|||
| if (__DEV__) { | |||
| var controlledValue = props.type === 'checkbox' ? props.checked : props.value; | |||
There was a problem hiding this comment.
checked is also valid for radio. We probably shouldn't be branching based on props.type. I think we can just do controlled = props.checked !== undefined || props.value != undefined
There was a problem hiding this comment.
it makes sense, I've updated the PR following your suggestion @jimfb
|
@TheBlasfem updated the pull request. |
|
|
||
| var initialValue = inst._wrapperState.initialChecked || inst._wrapperState.initialValue; | ||
| var defaultValue = props.defaultChecked || props.defaultValue; | ||
| var controlledValue = props.checked || props.value; |
There was a problem hiding this comment.
This can't be right I think? false || undefined => undefined and then later controlledValue !== undefined. Why not compute controlled instead of value? The value isn't used anywhere.
There was a problem hiding this comment.
i didn't notice that, thanks @syranide i've sent a fix
|
@TheBlasfem updated the pull request. |
1 similar comment
|
@TheBlasfem updated the pull request. |
|
@syranide i've sent a commit displaying the owner's name in the warning |
| warning( | ||
| false, | ||
| '%s is changing a uncontrolled input to be controlled. ' + | ||
| 'Input elements should not switch from uncontrolled to controlled(or viceversa). ' + |
There was a problem hiding this comment.
Missing space between "controlled(or"
|
A couple of nitpicks, and then I think this is ready to merge. Also, please squash the six commits into a single commit (eg. |
added controlled key to DEV warn for checkbox inputs warn for radio inputs compute controlled instead of value displaying owner name in warning displaying input type in warnings
|
@TheBlasfem updated the pull request. |
|
thanks for the comments @jimfb I've already applied the changes in a single commit squashed |
Warn when an input switches between controlled and uncontrolled
|
Thanks @TheBlasfem! |
the value in redux store shouldn't be affected. caused by small breaking change in react 15: facebook/react#5864
the value in redux store shouldn't be affected. caused by small breaking change in react 15: facebook/react#5864
…l (#15) * Fix react 15 console warning: 'value' prop on forms should not be null the value in redux store shouldn't be affected. caused by small breaking change in react 15: facebook/react#5864 * disable eslint complexity
Attempts to fix #5821
After using for a time React, I've decided to look into the source and start contributing, i hope this results helpful, I'll be pending for future improvements if are needed, thanks!