Merge pull request #2540 from spicyj/no-mutate-props

Warn when mutating props on a ReactElement

src/browser/ui/ReactMount.js

@@ -15,6 +15,7 @@ var DOMProperty = require('DOMProperty');
 var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
 var ReactCurrentOwner = require('ReactCurrentOwner');
 var ReactElement = require('ReactElement');
+var ReactElementValidator = require('ReactElementValidator');
 var ReactEmptyComponent = require('ReactEmptyComponent');
 var ReactInstanceHandles = require('ReactInstanceHandles');
 var ReactInstanceMap = require('ReactInstanceMap');
@@ -291,6 +292,10 @@ var ReactMount = {
       nextElement,
       container,
       callback) {
+    if (__DEV__) {
+      ReactElementValidator.checkAndWarnForMutatedProps(nextElement);
+    }
+
     var nextProps = nextElement.props;
     ReactMount.scrollMonitor(container, function() {
       prevComponent.replaceProps(nextProps, callback);

src/classic/element/ReactElement.js

@@ -14,6 +14,7 @@
 var ReactContext = require('ReactContext');
 var ReactCurrentOwner = require('ReactCurrentOwner');
 
+var assign = require('Object.assign');
 var warning = require('warning');
 
 var RESERVED_PROPS = {
@@ -44,8 +45,8 @@ function defineWarningProperty(object, key) {
     set: function(value) {
       warning(
         false,
-        'Don\'t set the ' + key + ' property of the component. ' +
-        'Mutate the existing props object instead.'
+        'Don\'t set the ' + key + ' property of the React element. Instead, ' +
+        'specify the correct value when initially creating the element.'
       );
       this._store[key] = value;
     }
@@ -106,7 +107,7 @@ var ReactElement = function(type, key, ref, owner, context, props) {
     // an external backing store so that we can freeze the whole object.
     // This can be replaced with a WeakMap once they are implemented in
     // commonly used development environments.
-    this._store = { props: props };
+    this._store = { props: props, originalProps: assign({}, props) };
 
     // To make comparing ReactElements easier for testing purposes, we make
     // the validation flag non-enumerable (where possible, which should

src/classic/element/ReactElementValidator.js

@@ -266,8 +266,78 @@ function checkPropTypes(componentName, propTypes, props, location) {
   }
 }
 
+var warnedPropsMutations = {};
+
+/**
+ * Warn about mutating props when setting `propName` on `element`.
+ *
+ * @param {string} propName The string key within props that was set
+ * @param {ReactElement} element
+ */
+function warnForPropsMutation(propName, element) {
+  var type = element.type;
+  var elementName = typeof type === 'string' ? type : type.displayName;
+  var ownerName = element._owner ?
+    element._owner.getPublicInstance().constructor.displayName : null;
+
+  var warningKey = propName + '|' + elementName + '|' + ownerName;
+  if (warnedPropsMutations.hasOwnProperty(warningKey)) {
+    return;
+  }
+  warnedPropsMutations[warningKey] = true;
+
+  var elementInfo = '';
+  if (elementName) {
+    elementInfo = ' <' + elementName + ' />';
+  }
+  var ownerInfo = '';
+  if (ownerName) {
+    ownerInfo = ' The element was created by ' + ownerName + '.';
+  }
+
+  warning(
+    false,
+    'Don\'t set .props.' + propName + ' of the React component' +
+    elementInfo + '. Instead, specify the correct value when ' +
+    'initially creating the element.' + ownerInfo
+  );
+}
+
+/**
+ * Given an element, check if its props have been mutated since element
+ * creation (or the last call to this function). In particular, check if any
+ * new props have been added, which we can't directly catch by defining warning
+ * properties on the props object.
+ *
+ * @param {ReactElement} element
+ */
+function checkAndWarnForMutatedProps(element) {
+  if (!element._store) {
+    // Element was created using `new ReactElement` directly or with
+    // `ReactElement.createElement`; skip mutation checking
+    return;
+  }
+
+  var originalProps = element._store.originalProps;
+  var props = element.props;
+
+  for (var propName in props) {
+    if (props.hasOwnProperty(propName)) {
+      if (!originalProps.hasOwnProperty(propName) ||
+          originalProps[propName] !== props[propName]) {
+        warnForPropsMutation(propName, element);
+
+        // Copy over the new value so that the two props objects match again
+        originalProps[propName] = props[propName];
+      }
+    }
+  }
+}
+
 var ReactElementValidator = {
 
+  checkAndWarnForMutatedProps: checkAndWarnForMutatedProps,
+
   createElement: function(type, props, children) {
     // We warn in this case but don't throw. We expect the element creation to
     // succeed and there will likely be errors in render.

src/classic/element/__tests__/ReactElement-test.js

@@ -269,4 +269,83 @@ describe('ReactElement', function() {
     expect(inst2.props.prop).toBe(null);
   });
 
+  it('warns when changing a prop after element creation', function() {
+    spyOn(console, 'warn');
+    var Outer = React.createClass({
+      render: function() {
+        var el = <div className="moo" />;
+
+        // This assignment warns but should still work for now.
+        el.props.className = 'quack';
+        expect(el.props.className).toBe('quack');
+
+        return el;
+      }
+    });
+    var outer = ReactTestUtils.renderIntoDocument(<Outer color="orange" />);
+    expect(outer.getDOMNode().className).toBe('quack');
+
+    expect(console.warn.argsForCall.length).toBe(1);
+    expect(console.warn.argsForCall[0][0]).toContain(
+      'Don\'t set .props.className of the React component <div />.'
+    );
+    expect(console.warn.argsForCall[0][0]).toContain(
+      'The element was created by Outer.'
+    );
+
+    console.warn.reset();
+
+    // This also warns (just once per key/type pair)
+    outer.props.color = 'green';
+    outer.forceUpdate();
+    outer.props.color = 'purple';
+    outer.forceUpdate();
+
+    expect(console.warn.argsForCall.length).toBe(1);
+    expect(console.warn.argsForCall[0][0]).toContain(
+      'Don\'t set .props.color of the React component <Outer />.'
+    );
+  });
+
+  it('warns when adding a prop after element creation', function() {
+    spyOn(console, 'warn');
+    var el = document.createElement('div');
+    var Outer = React.createClass({
+      getDefaultProps: () => ({sound: 'meow'}),
+      render: function() {
+        var el = <div>{this.props.sound}</div>;
+
+        // This assignment doesn't warn immediately (because we can't) but it
+        // warns upon mount.
+        el.props.className = 'quack';
+        expect(el.props.className).toBe('quack');
+
+        return el;
+      }
+    });
+    var outer = React.render(<Outer />, el);
+    expect(outer.getDOMNode().textContent).toBe('meow');
+    expect(outer.getDOMNode().className).toBe('quack');
+
+    expect(console.warn.argsForCall.length).toBe(1);
+    expect(console.warn.argsForCall[0][0]).toContain(
+      'Don\'t set .props.className of the React component <div />.'
+    );
+    expect(console.warn.argsForCall[0][0]).toContain(
+      'The element was created by Outer.'
+    );
+
+    console.warn.reset();
+
+    var newOuterEl = <Outer />;
+    newOuterEl.props.sound = 'oink';
+    outer = React.render(newOuterEl, el);
+    expect(outer.getDOMNode().textContent).toBe('oink');
+    expect(outer.getDOMNode().className).toBe('quack');
+
+    expect(console.warn.argsForCall.length).toBe(1);
+    expect(console.warn.argsForCall[0][0]).toContain(
+      'Don\'t set .props.sound of the React component <Outer />.'
+    );
+  });
 });

src/core/ReactComponent.js

@@ -11,6 +11,7 @@
 
 'use strict';
 
+var ReactElementValidator = require('ReactElementValidator');
 var ReactOwner = require('ReactOwner');
 var ReactRef = require('ReactRef');
 
@@ -110,6 +111,10 @@ var ReactComponent = {
      * @internal
      */
     mountComponent: function(rootID, transaction, context) {
+      if (__DEV__) {
+        ReactElementValidator.checkAndWarnForMutatedProps(this._currentElement);
+      }
+
       var ref = this._currentElement.ref;
       if (ref != null) {
         var owner = this._currentElement._owner;
@@ -144,6 +149,10 @@ var ReactComponent = {
      * @internal
      */
     updateComponent: function(transaction, prevElement, nextElement, context) {
+      if (__DEV__) {
+        ReactElementValidator.checkAndWarnForMutatedProps(nextElement);
+      }
+
       // If either the owner or a `ref` has changed, make sure the newest owner
       // has stored a reference to `this`, and the previous owner (if different)
       // has forgotten the reference to `this`. We use the element instead