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.

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

Warn when mutating props on a ReactElement #2540

Merged
merged 1 commit into from
Jan 14, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions src/browser/ui/ReactMount.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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);
Expand Down
7 changes: 4 additions & 3 deletions src/classic/element/ReactElement.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
var ReactContext = require('ReactContext');
var ReactCurrentOwner = require('ReactCurrentOwner');

var assign = require('Object.assign');
var warning = require('warning');

var RESERVED_PROPS = {
Expand Down Expand Up @@ -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;
}
Expand Down Expand Up @@ -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
Expand Down
70 changes: 70 additions & 0 deletions src/classic/element/ReactElementValidator.js
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
79 changes: 79 additions & 0 deletions src/classic/element/__tests__/ReactElement-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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 />.'
);
});
});
9 changes: 9 additions & 0 deletions src/core/ReactComponent.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@

"use strict";

var ReactElementValidator = require('ReactElementValidator');
var ReactOwner = require('ReactOwner');
var ReactRef = require('ReactRef');

Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down