Skip to content
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

Merged
merged 1 commit into from Feb 19, 2016

Conversation

Projects
None yet
5 participants
@truongduy134
Copy link
Contributor

commented Feb 8, 2016

Fix #5929. Resolve to default props when config key is set to undefined in cloneElement

var Foo = React.createClass({
   getDefaultProps: function() {
      return {foo: 'bar'};
   },
   render: function() {
      return React.createElement('span', null, this.props.prop);
   },
});

clonedElement = React.cloneElement(React.createElement(Foo, {foo: 'foo'}), {foo: undefined});
console.log(element.props.foo); // Should be 'bar' instead of undefined
@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 8, 2016

@truongduy134 updated the pull request.

@truongduy134 truongduy134 force-pushed the truongduy134:clone-element branch from 606f772 to 190950f Feb 8, 2016

@@ -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) {

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 8, 2016

Contributor

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.

This comment has been minimized.

Copy link
@truongduy134

truongduy134 Feb 8, 2016

Author Contributor

Ah I see. Will update the behavior and test cases. Thanks @jimfb

@truongduy134 truongduy134 force-pushed the truongduy134:clone-element branch from 190950f to 73ffcd2 Feb 8, 2016

@truongduy134 truongduy134 changed the title Fix #5929. Do not override props with undefined in cloneElement Fix #5929. Resolve to default props when config key is set to undefined in cloneElement Feb 8, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 8, 2016

@truongduy134 updated the pull request.

// Resolve default props
if (element.type && element.type.defaultProps) {
var defaultProps = element.type.defaultProps;
for (propName in defaultProps) {

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 8, 2016

Contributor

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.

return {prop: 'testKey'};
},
render: function() {
return React.createElement('span', null, this.props.prop);

This comment has been minimized.

Copy link
@zpao

zpao Feb 8, 2016

Member

Please use JSX

@truongduy134 truongduy134 force-pushed the truongduy134:clone-element branch from 73ffcd2 to c5254da Feb 9, 2016

@truongduy134

This comment has been minimized.

Copy link
Contributor Author

commented Feb 9, 2016

@jimfb , @zpao: Thank you for the reviews. Addressed your comments

@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 9, 2016

@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) {

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 9, 2016

Contributor

&& defaultProps !== undefined

@@ -243,10 +243,19 @@ ReactElement.cloneElement = function(element, config, children) {
key = '' + config.key;
}
// Remaining properties override existing props
var defaultProps = {};

This comment has been minimized.

Copy link
@jimfb

jimfb Feb 9, 2016

Contributor

We are doing an extra/unnecessary allocation here, which creates extra gc pressure. See below.

This comment has been minimized.

Copy link
@truongduy134

truongduy134 Feb 9, 2016

Author Contributor

Thanks @jimfb . I usually have this allocation to avoid undefined checking, but not realize it has bad effect on gc. Updated my PR

Resolve default prop values in cloneElement
In cloneElement, when key in input config is set to undefined, the associated
prop value should be resolved to default prop values

@truongduy134 truongduy134 force-pushed the truongduy134:clone-element branch from c5254da to 48ded23 Feb 9, 2016

@facebook-github-bot

This comment has been minimized.

Copy link

commented Feb 9, 2016

@truongduy134 updated the pull request.

@jimfb jimfb self-assigned this Feb 9, 2016

@sebmarkbage

This comment has been minimized.

Copy link
Member

commented Feb 9, 2016

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.

@truongduy134

This comment has been minimized.

Copy link
Contributor Author

commented Feb 10, 2016

@jimfb : Do we need to add a warning message ?

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 10, 2016

@truongduy134 I'll need to do some checks internally and report back. It will take at least a few days.

@truongduy134

This comment has been minimized.

Copy link
Contributor Author

commented Feb 19, 2016

@jimfb : Do we have any update on this PR?

@jimfb

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2016

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.

@jimfb jimfb closed this Feb 19, 2016

@jimfb jimfb reopened this Feb 19, 2016

jimfb added a commit that referenced this pull request Feb 19, 2016

Merge pull request #5997 from truongduy134/clone-element
Fix #5929. Resolve to default props when config key is set to undefined in cloneElement

@jimfb jimfb merged commit 70b5eda into facebook:master Feb 19, 2016

1 check was pending

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details

@renovate renovate bot referenced this pull request Feb 2, 2018

Open

Update dependency react to v0.14.9 #29

0 of 1 task complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.