From 42c14b8078e75b19ad42e88772f01e2b7c6fc93f Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Wed, 20 Nov 2013 01:12:39 -0800 Subject: [PATCH] Don't mutate passed-in props 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.) --- src/core/ReactCompositeComponent.js | 28 ++++++++++++------- .../__tests__/ReactCompositeComponent-test.js | 20 +++++++++++++ 2 files changed, 38 insertions(+), 10 deletions(-) diff --git a/src/core/ReactCompositeComponent.js b/src/core/ReactCompositeComponent.js index 6c61396d47f..f1581b54135 100644 --- a/src/core/ReactCompositeComponent.js +++ b/src/core/ReactCompositeComponent.js @@ -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(); @@ -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; + 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; }, /** @@ -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; diff --git a/src/core/__tests__/ReactCompositeComponent-test.js b/src/core/__tests__/ReactCompositeComponent-test.js index 3afd659d8da..fd6d355932e 100644 --- a/src/core/__tests__/ReactCompositeComponent-test.js +++ b/src/core/__tests__/ReactCompositeComponent-test.js @@ -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 ; + } + }); + + 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},