-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!