Stop Mutating Merged Lifecycle Results
Summary: Currently, `ReactClass` mutates values returned by `getDefaultProps`, `getInitialState`, and `getChildContext`. This is bad because the objects may, for example, be cached and re-used across instances of a React component. This changes `ReactClass` to instead create a new object. In return for allocating a new object, I've replaced `mapObject` with a `for ... in` so that we are no longer allocating an unused object. Fair trade, IMO. Test Plan: Ran unit tests successfully: ``` npm run jest ``` Conflicts: src/core/ReactCompositeComponent.js Conflicts: src/class/ReactClass.js
yungsters committed
Nov 18, 2014 at 10:00 UTC
8e1504628342322488e6c0a9fa32b58ae8fd995f
2 files changed
+41
-17
src/class/ReactClass.js
+20
-16
@@ -22,7 +22,6 @@ var invariant = require('invariant');
22
var keyMirror = require('keyMirror');
23
var keyOf = require('keyOf');
24
var monitorCodeUse = require('monitorCodeUse');
25
-var mapObject = require('mapObject');
25
var warning = require('warning');
26
27
var MIXINS_KEY = keyOf({mixins: null});
@@ -566,24 +565,26 @@ function mixStaticSpecIntoComponent(Constructor, statics) {
565
* @param {object} two The second object
566
* @return {object} one after it has been mutated to contain everything in two.
567
*/
569
-function mergeObjectsWithNoDuplicateKeys(one, two) {
568
+function mergeIntoWithNoDuplicateKeys(one, two) {
569
invariant(
570
one && two && typeof one === 'object' && typeof two === 'object',
572
- 'mergeObjectsWithNoDuplicateKeys(): Cannot merge non-objects'
571
+ 'mergeIntoWithNoDuplicateKeys(): Cannot merge non-objects.'
572
);
573
575
- mapObject(two, function(value, key) {
576
- invariant(
577
- one[key] === undefined,
578
- 'mergeObjectsWithNoDuplicateKeys(): ' +
579
- 'Tried to merge two objects with the same key: `%s`. This conflict ' +
580
- 'may be due to a mixin; in particular, this may be caused by two ' +
581
- 'getInitialState() or getDefaultProps() methods returning objects ' +
582
- 'with clashing keys.',
583
- key
584
- );
585
- one[key] = value;
586
- });
574
+ for (var key in two) {
575
+ if (two.hasOwnProperty(key)) {
576
+ invariant(
577
+ one[key] === undefined,
578
+ 'mergeIntoWithNoDuplicateKeys(): ' +
579
+ 'Tried to merge two objects with the same key: `%s`. This conflict ' +
580
+ 'may be due to a mixin; in particular, this may be caused by two ' +
581
+ 'getInitialState() or getDefaultProps() methods returning objects ' +
582
+ 'with clashing keys.',
583
+ key
584
+ );
585
+ one[key] = two[key];
586
+ }
587
+ }
588
return one;
589
}
590
@@ -604,7 +605,10 @@ function createMergedResultFunction(one, two) {
605
} else if (b == null) {
606
return a;
607
}
607
- return mergeObjectsWithNoDuplicateKeys(a, b);
608
+ var c = {};
609
+ mergeIntoWithNoDuplicateKeys(c, a);
610
+ mergeIntoWithNoDuplicateKeys(c, b);
611
+ return c;
612
};
613
}
614
src/core/__tests__/ReactCompositeComponent-test.js
+21
-1
@@ -586,7 +586,7 @@ describe('ReactCompositeComponent', function() {
586
expect(function() {
587
instance = ReactTestUtils.renderIntoDocument(instance);
588
}).toThrow(
589
- 'Invariant Violation: mergeObjectsWithNoDuplicateKeys(): ' +
589
+ 'Invariant Violation: mergeIntoWithNoDuplicateKeys(): ' +
590
'Tried to merge two objects with the same key: `x`. This conflict ' +
591
'may be due to a mixin; in particular, this may be caused by two ' +
592
'getInitialState() or getDefaultProps() methods returning objects ' +
@@ -594,6 +594,26 @@ describe('ReactCompositeComponent', function() {
594
);
595
});
596
597
+ it('should not mutate objects returned by getInitialState()', function() {
598
+ var Mixin = {
599
+ getInitialState: function() {
600
+ return Object.freeze({mixin: true});
601
+ }
602
+ };
603
+ var Component = React.createClass({
604
+ mixins: [Mixin],
605
+ getInitialState: function() {
606
+ return Object.freeze({component: true});
607
+ },
608
+ render: function() {
609
+ return <span />;
610
+ }
611
+ });
612
+ expect(() => {
613
+ ReactTestUtils.renderIntoDocument(<Component />);
614
+ }).not.toThrow();
615
+ });
616
+
617
it('should work with object getInitialState() return values', function() {
618
var Component = React.createClass({
619
getInitialState: function() {