-
Notifications
You must be signed in to change notification settings - Fork 46.8k
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
Fix #5929. Resolve to default props when config key is set to undefined in cloneElement #5997
Conversation
@truongduy134 updated the pull request. |
606f772
to
190950f
Compare
@@ -245,7 +245,8 @@ ReactElement.cloneElement = function(element, config, children) { | |||
// Remaining properties override existing props | |||
for (propName in config) { | |||
if (config.hasOwnProperty(propName) && | |||
!RESERVED_PROPS.hasOwnProperty(propName)) { | |||
!RESERVED_PROPS.hasOwnProperty(propName) && | |||
config[propName] !== 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.
This is wrong, because setting the value to undefined SHOULD change the element if the prop was set to a non-default value when the element was created. The desired behavior would be to use the prop's default 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.
Ah I see. Will update the behavior and test cases. Thanks @jimfb
190950f
to
73ffcd2
Compare
@truongduy134 updated the pull request. |
// Resolve default props | ||
if (element.type && element.type.defaultProps) { | ||
var defaultProps = element.type.defaultProps; | ||
for (propName in defaultProps) { |
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.
We probably don't need to introduce a loop here, since we know the only way this could happen is if the config prop is undefined. We can just add an undefined check to the existing loop above.
73ffcd2
to
c5254da
Compare
@truongduy134 updated the pull request. |
for (propName in config) { | ||
if (config.hasOwnProperty(propName) && | ||
!RESERVED_PROPS.hasOwnProperty(propName)) { | ||
props[propName] = config[propName]; | ||
if (config[propName] === 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.
&& defaultProps !== undefined
In cloneElement, when key in input config is set to undefined, the associated prop value should be resolved to default prop values
c5254da
to
48ded23
Compare
@truongduy134 updated the pull request. |
This is a breaking change. We might need a warning upgrade path. We should investigate first to know how likely this is to break and how bad it could be. |
@jimfb : Do we need to add a warning message ? |
@truongduy134 I'll need to do some checks internally and report back. It will take at least a few days. |
@jimfb : Do we have any update on this PR? |
Yep, we were just looking at the results, it looks like the impact is low enough that we are comfortable with it. Will require one minor change internally when we sync. |
Fix #5929. Resolve to default props when config key is set to undefined in cloneElement
Fix #5929. Resolve to default props when config key is set to undefined in cloneElement