-
Notifications
You must be signed in to change notification settings - Fork 50k
Don't mutate passed-in props #576
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
Conversation
Depends on facebook#575; fixes facebook#570. Now we'll be in trouble if someone tries to share objects between calls to getDefaultProps but that was already true of getInitialState and I haven't heard any complaints there. This is the same number of allocations as before; we're just copying props in the other direction. (In any case, the copy happens only on mount and there are a couple dozen costlier things we're doing already at that time.)
|
+1. Can you explain what you mean by "we'll be in trouble if someone tries to share objects between calls to getDefaultProps but that was already true of getInitialState"? Like modifying a global object in the |
|
Yep, if you do then all components would share a |
|
pulling this in |
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.
did you mean to clone this._defaultProps here? this mutates the default prop object which gets reused on each pass.
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.
In my original PR comment: "Now we'll be in trouble if someone tries to share objects between calls to getDefaultProps but that was already true of getInitialState and I haven't heard any complaints there."
But we can copy if that's a problem?
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.
MyComponent = React.createClass({
getDefaultProps: function() { return {}; },
render: function() { ... }
};
// render <MyComponent disabled={true} />
// update with <MyComponent />we'll see that disabled is true for the second pass because this._defaultProps has been modified.
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.
Ahh, you're right.
This reverts facebook/react#576 This approach mutates the default props for the instance on each update, which causes incorrect behavior. discussed with @balpert. can look into cloning but this unbreaks.
Depends on #575; fixes #570. Now we'll be in trouble if someone tries to share objects between calls to getDefaultProps but that was already true of getInitialState and I haven't heard any complaints there.
This is the same number of allocations as before; we're just copying props in the other direction. (In any case, the copy happens only on mount and there are a couple dozen costlier things we're doing already at that time.)