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.

Sign up for GitHub

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

Jump to bottom

Don't mutate passed-in props #576

Merged
merged 1 commit into from
Dec 2, 2013
Merged

Don't mutate passed-in props #576

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
28 changes: 18 additions & 10 deletions src/core/ReactCompositeComponent.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ var ReactCompositeComponentMixin = {
this._compositeLifeCycleState = CompositeLifeCycle.MOUNTING;

this._defaultProps = this.getDefaultProps ? this.getDefaultProps() : null;
this._processProps(this.props);
this.props = this._processProps(this.props);

if (this.__reactAutoBindMap) {
this._bindAutoBindMethods();
Expand Down Expand Up @@ -757,22 +757,31 @@ var ReactCompositeComponentMixin = {

/**
* Processes props by setting default values for unspecified props and
* asserting that the props are valid.
* asserting that the props are valid. Does not mutate its argument; returns
* a new props object with defaults merged in.
*
* @param {object} props
* @param {object} newProps
* @return {object}
* @private
*/
_processProps: function(props) {
var defaultProps = this._defaultProps;
for (var propName in defaultProps) {
if (typeof props[propName] === 'undefined') {
props[propName] = defaultProps[propName];
_processProps: function(newProps) {
var props = this._defaultProps;
Copy link
Contributor

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.

Copy link
Collaborator Author

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?

Copy link
Contributor

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh, you're right.

if (props) {
// To avoid mutating the props that are passed into the constructor, we
// copy onto the object returned by getDefaultProps instead.
for (var propName in newProps) {
if (typeof newProps[propName] !== 'undefined') {
props[propName] = newProps[propName];
}
}
} else {
props = newProps;
}
var propTypes = this.constructor.propTypes;
if (propTypes) {
this._checkPropTypes(propTypes, props, ReactPropTypeLocations.prop);
}
return props;
},

/**
Expand Down Expand Up @@ -821,8 +830,7 @@ var ReactCompositeComponentMixin = {

var nextProps = this.props;
if (this._pendingProps != null) {
nextProps = this._pendingProps;
this._processProps(nextProps);
nextProps = this._processProps(this._pendingProps);
this._pendingProps = null;

this._compositeLifeCycleState = CompositeLifeCycle.RECEIVING_PROPS;
Expand Down
20 changes: 20 additions & 0 deletions src/core/__tests__/ReactCompositeComponent-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -213,6 +213,26 @@ describe('ReactCompositeComponent', function() {
reactComponentExpect(instance3).scalarPropsEqual({key: null});
});

it('should not mutate passed-in props object', function() {
var Component = React.createClass({
getDefaultProps: function() {
return {key: 'testKey'};
},
render: function() {
return <span />;
}
});

var inputProps = {};
var instance1 = Component(inputProps);
ReactTestUtils.renderIntoDocument(instance1);
expect(instance1.props.key).toBe('testKey');

// We don't mutate the input, just in case the caller wants to do something
// with it after using it to instantiate a component
expect(inputProps.key).not.toBeDefined();
});

it('should normalize props with default values', function() {
var Component = React.createClass({
propTypes: {key: ReactPropTypes.string.isRequired},
Expand Down