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.)
Ben Alpert committed
Nov 20, 2013 at 01:12 UTC
42c14b8078e75b19ad42e88772f01e2b7c6fc93f
2 files changed
+38
-10
src/core/ReactCompositeComponent.js
+18
-10
@@ -589,7 +589,7 @@ var ReactCompositeComponentMixin = {
589
this._compositeLifeCycleState = CompositeLifeCycle.MOUNTING;
590
591
this._defaultProps = this.getDefaultProps ? this.getDefaultProps() : null;
592
- this._processProps(this.props);
592
+ this.props = this._processProps(this.props);
593
594
if (this.__reactAutoBindMap) {
595
this._bindAutoBindMethods();
@@ -757,22 +757,31 @@ var ReactCompositeComponentMixin = {
757
758
/**
759
* Processes props by setting default values for unspecified props and
760
- * asserting that the props are valid.
760
+ * asserting that the props are valid. Does not mutate its argument; returns
761
+ * a new props object with defaults merged in.
762
*
762
- * @param {object} props
763
+ * @param {object} newProps
764
+ * @return {object}
765
* @private
766
*/
765
- _processProps: function(props) {
766
- var defaultProps = this._defaultProps;
767
- for (var propName in defaultProps) {
768
- if (typeof props[propName] === 'undefined') {
769
- props[propName] = defaultProps[propName];
767
+ _processProps: function(newProps) {
768
+ var props = this._defaultProps;
769
+ if (props) {
770
+ // To avoid mutating the props that are passed into the constructor, we
771
+ // copy onto the object returned by getDefaultProps instead.
772
+ for (var propName in newProps) {
773
+ if (typeof newProps[propName] !== 'undefined') {
774
+ props[propName] = newProps[propName];
775
+ }
776
}
777
+ } else {
778
+ props = newProps;
779
}
780
var propTypes = this.constructor.propTypes;
781
if (propTypes) {
782
this._checkPropTypes(propTypes, props, ReactPropTypeLocations.prop);
783
}
784
+ return props;
785
},
786
787
/**
@@ -821,8 +830,7 @@ var ReactCompositeComponentMixin = {
830
831
var nextProps = this.props;
832
if (this._pendingProps != null) {
824
- nextProps = this._pendingProps;
825
- this._processProps(nextProps);
833
+ nextProps = this._processProps(this._pendingProps);
834
this._pendingProps = null;
835
836
this._compositeLifeCycleState = CompositeLifeCycle.RECEIVING_PROPS;
src/core/__tests__/ReactCompositeComponent-test.js
+20
@@ -213,6 +213,26 @@ describe('ReactCompositeComponent', function() {
213
reactComponentExpect(instance3).scalarPropsEqual({key: null});
214
});
215
216
+ it('should not mutate passed-in props object', function() {
217
+ var Component = React.createClass({
218
+ getDefaultProps: function() {
219
+ return {key: 'testKey'};
220
+ },
221
+ render: function() {
222
+ return <span />;
223
+ }
224
+ });
225
+
226
+ var inputProps = {};
227
+ var instance1 = Component(inputProps);
228
+ ReactTestUtils.renderIntoDocument(instance1);
229
+ expect(instance1.props.key).toBe('testKey');
230
+
231
+ // We don't mutate the input, just in case the caller wants to do something
232
+ // with it after using it to instantiate a component
233
+ expect(inputProps.key).not.toBeDefined();
234
+ });
235
+
236
it('should normalize props with default values', function() {
237
var Component = React.createClass({
238
propTypes: {key: ReactPropTypes.string.isRequired},