Skip to content

Commit 8d5838a

Browse files
committed
Merge pull request #2540 from spicyj/no-mutate-props
Warn when mutating props on a ReactElement
2 parents 892f0a5 + a5aacb9 commit 8d5838a

File tree

5 files changed

+167
-3
lines changed

5 files changed

+167
-3
lines changed

src/browser/ui/ReactMount.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ var DOMProperty = require('DOMProperty');
1515
var ReactBrowserEventEmitter = require('ReactBrowserEventEmitter');
1616
var ReactCurrentOwner = require('ReactCurrentOwner');
1717
var ReactElement = require('ReactElement');
18+
var ReactElementValidator = require('ReactElementValidator');
1819
var ReactEmptyComponent = require('ReactEmptyComponent');
1920
var ReactInstanceHandles = require('ReactInstanceHandles');
2021
var ReactInstanceMap = require('ReactInstanceMap');
@@ -291,6 +292,10 @@ var ReactMount = {
291292
nextElement,
292293
container,
293294
callback) {
295+
if (__DEV__) {
296+
ReactElementValidator.checkAndWarnForMutatedProps(nextElement);
297+
}
298+
294299
var nextProps = nextElement.props;
295300
ReactMount.scrollMonitor(container, function() {
296301
prevComponent.replaceProps(nextProps, callback);

src/classic/element/ReactElement.js

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@
1414
var ReactContext = require('ReactContext');
1515
var ReactCurrentOwner = require('ReactCurrentOwner');
1616

17+
var assign = require('Object.assign');
1718
var warning = require('warning');
1819

1920
var RESERVED_PROPS = {
@@ -44,8 +45,8 @@ function defineWarningProperty(object, key) {
4445
set: function(value) {
4546
warning(
4647
false,
47-
'Don\'t set the ' + key + ' property of the component. ' +
48-
'Mutate the existing props object instead.'
48+
'Don\'t set the ' + key + ' property of the React element. Instead, ' +
49+
'specify the correct value when initially creating the element.'
4950
);
5051
this._store[key] = value;
5152
}
@@ -106,7 +107,7 @@ var ReactElement = function(type, key, ref, owner, context, props) {
106107
// an external backing store so that we can freeze the whole object.
107108
// This can be replaced with a WeakMap once they are implemented in
108109
// commonly used development environments.
109-
this._store = { props: props };
110+
this._store = { props: props, originalProps: assign({}, props) };
110111

111112
// To make comparing ReactElements easier for testing purposes, we make
112113
// the validation flag non-enumerable (where possible, which should

src/classic/element/ReactElementValidator.js

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,8 +266,78 @@ function checkPropTypes(componentName, propTypes, props, location) {
266266
}
267267
}
268268

269+
var warnedPropsMutations = {};
270+
271+
/**
272+
* Warn about mutating props when setting `propName` on `element`.
273+
*
274+
* @param {string} propName The string key within props that was set
275+
* @param {ReactElement} element
276+
*/
277+
function warnForPropsMutation(propName, element) {
278+
var type = element.type;
279+
var elementName = typeof type === 'string' ? type : type.displayName;
280+
var ownerName = element._owner ?
281+
element._owner.getPublicInstance().constructor.displayName : null;
282+
283+
var warningKey = propName + '|' + elementName + '|' + ownerName;
284+
if (warnedPropsMutations.hasOwnProperty(warningKey)) {
285+
return;
286+
}
287+
warnedPropsMutations[warningKey] = true;
288+
289+
var elementInfo = '';
290+
if (elementName) {
291+
elementInfo = ' <' + elementName + ' />';
292+
}
293+
var ownerInfo = '';
294+
if (ownerName) {
295+
ownerInfo = ' The element was created by ' + ownerName + '.';
296+
}
297+
298+
warning(
299+
false,
300+
'Don\'t set .props.' + propName + ' of the React component' +
301+
elementInfo + '. Instead, specify the correct value when ' +
302+
'initially creating the element.' + ownerInfo
303+
);
304+
}
305+
306+
/**
307+
* Given an element, check if its props have been mutated since element
308+
* creation (or the last call to this function). In particular, check if any
309+
* new props have been added, which we can't directly catch by defining warning
310+
* properties on the props object.
311+
*
312+
* @param {ReactElement} element
313+
*/
314+
function checkAndWarnForMutatedProps(element) {
315+
if (!element._store) {
316+
// Element was created using `new ReactElement` directly or with
317+
// `ReactElement.createElement`; skip mutation checking
318+
return;
319+
}
320+
321+
var originalProps = element._store.originalProps;
322+
var props = element.props;
323+
324+
for (var propName in props) {
325+
if (props.hasOwnProperty(propName)) {
326+
if (!originalProps.hasOwnProperty(propName) ||
327+
originalProps[propName] !== props[propName]) {
328+
warnForPropsMutation(propName, element);
329+
330+
// Copy over the new value so that the two props objects match again
331+
originalProps[propName] = props[propName];
332+
}
333+
}
334+
}
335+
}
336+
269337
var ReactElementValidator = {
270338

339+
checkAndWarnForMutatedProps: checkAndWarnForMutatedProps,
340+
271341
createElement: function(type, props, children) {
272342
// We warn in this case but don't throw. We expect the element creation to
273343
// succeed and there will likely be errors in render.

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

Lines changed: 79 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -269,4 +269,83 @@ describe('ReactElement', function() {
269269
expect(inst2.props.prop).toBe(null);
270270
});
271271

272+
it('warns when changing a prop after element creation', function() {
273+
spyOn(console, 'warn');
274+
var Outer = React.createClass({
275+
render: function() {
276+
var el = <div className="moo" />;
277+
278+
// This assignment warns but should still work for now.
279+
el.props.className = 'quack';
280+
expect(el.props.className).toBe('quack');
281+
282+
return el;
283+
}
284+
});
285+
var outer = ReactTestUtils.renderIntoDocument(<Outer color="orange" />);
286+
expect(outer.getDOMNode().className).toBe('quack');
287+
288+
expect(console.warn.argsForCall.length).toBe(1);
289+
expect(console.warn.argsForCall[0][0]).toContain(
290+
'Don\'t set .props.className of the React component <div />.'
291+
);
292+
expect(console.warn.argsForCall[0][0]).toContain(
293+
'The element was created by Outer.'
294+
);
295+
296+
console.warn.reset();
297+
298+
// This also warns (just once per key/type pair)
299+
outer.props.color = 'green';
300+
outer.forceUpdate();
301+
outer.props.color = 'purple';
302+
outer.forceUpdate();
303+
304+
expect(console.warn.argsForCall.length).toBe(1);
305+
expect(console.warn.argsForCall[0][0]).toContain(
306+
'Don\'t set .props.color of the React component <Outer />.'
307+
);
308+
});
309+
310+
it('warns when adding a prop after element creation', function() {
311+
spyOn(console, 'warn');
312+
var el = document.createElement('div');
313+
var Outer = React.createClass({
314+
getDefaultProps: () => ({sound: 'meow'}),
315+
render: function() {
316+
var el = <div>{this.props.sound}</div>;
317+
318+
// This assignment doesn't warn immediately (because we can't) but it
319+
// warns upon mount.
320+
el.props.className = 'quack';
321+
expect(el.props.className).toBe('quack');
322+
323+
return el;
324+
}
325+
});
326+
var outer = React.render(<Outer />, el);
327+
expect(outer.getDOMNode().textContent).toBe('meow');
328+
expect(outer.getDOMNode().className).toBe('quack');
329+
330+
expect(console.warn.argsForCall.length).toBe(1);
331+
expect(console.warn.argsForCall[0][0]).toContain(
332+
'Don\'t set .props.className of the React component <div />.'
333+
);
334+
expect(console.warn.argsForCall[0][0]).toContain(
335+
'The element was created by Outer.'
336+
);
337+
338+
console.warn.reset();
339+
340+
var newOuterEl = <Outer />;
341+
newOuterEl.props.sound = 'oink';
342+
outer = React.render(newOuterEl, el);
343+
expect(outer.getDOMNode().textContent).toBe('oink');
344+
expect(outer.getDOMNode().className).toBe('quack');
345+
346+
expect(console.warn.argsForCall.length).toBe(1);
347+
expect(console.warn.argsForCall[0][0]).toContain(
348+
'Don\'t set .props.sound of the React component <Outer />.'
349+
);
350+
});
272351
});

src/core/ReactComponent.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
'use strict';
1313

14+
var ReactElementValidator = require('ReactElementValidator');
1415
var ReactOwner = require('ReactOwner');
1516
var ReactRef = require('ReactRef');
1617

@@ -110,6 +111,10 @@ var ReactComponent = {
110111
* @internal
111112
*/
112113
mountComponent: function(rootID, transaction, context) {
114+
if (__DEV__) {
115+
ReactElementValidator.checkAndWarnForMutatedProps(this._currentElement);
116+
}
117+
113118
var ref = this._currentElement.ref;
114119
if (ref != null) {
115120
var owner = this._currentElement._owner;
@@ -144,6 +149,10 @@ var ReactComponent = {
144149
* @internal
145150
*/
146151
updateComponent: function(transaction, prevElement, nextElement, context) {
152+
if (__DEV__) {
153+
ReactElementValidator.checkAndWarnForMutatedProps(nextElement);
154+
}
155+
147156
// If either the owner or a `ref` has changed, make sure the newest owner
148157
// has stored a reference to `this`, and the previous owner (if different)
149158
// has forgotten the reference to `this`. We use the element instead

0 commit comments

Comments
 (0)