From 8e1504628342322488e6c0a9fa32b58ae8fd995f Mon Sep 17 00:00:00 2001 From: yungsters Date: Tue, 18 Nov 2014 10:00:22 -0800 Subject: [PATCH] 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 --- src/class/ReactClass.js | 36 ++++++++++--------- .../__tests__/ReactCompositeComponent-test.js | 22 +++++++++++- 2 files changed, 41 insertions(+), 17 deletions(-) diff --git a/src/class/ReactClass.js b/src/class/ReactClass.js index bae11f56219..b38bc4c72ce 100644 --- a/src/class/ReactClass.js +++ b/src/class/ReactClass.js @@ -22,7 +22,6 @@ var invariant = require('invariant'); var keyMirror = require('keyMirror'); var keyOf = require('keyOf'); var monitorCodeUse = require('monitorCodeUse'); -var mapObject = require('mapObject'); var warning = require('warning'); var MIXINS_KEY = keyOf({mixins: null}); @@ -566,24 +565,26 @@ function mixStaticSpecIntoComponent(Constructor, statics) { * @param {object} two The second object * @return {object} one after it has been mutated to contain everything in two. */ -function mergeObjectsWithNoDuplicateKeys(one, two) { +function mergeIntoWithNoDuplicateKeys(one, two) { invariant( one && two && typeof one === 'object' && typeof two === 'object', - 'mergeObjectsWithNoDuplicateKeys(): Cannot merge non-objects' + 'mergeIntoWithNoDuplicateKeys(): Cannot merge non-objects.' ); - mapObject(two, function(value, key) { - invariant( - one[key] === undefined, - 'mergeObjectsWithNoDuplicateKeys(): ' + - 'Tried to merge two objects with the same key: `%s`. This conflict ' + - 'may be due to a mixin; in particular, this may be caused by two ' + - 'getInitialState() or getDefaultProps() methods returning objects ' + - 'with clashing keys.', - key - ); - one[key] = value; - }); + for (var key in two) { + if (two.hasOwnProperty(key)) { + invariant( + one[key] === undefined, + 'mergeIntoWithNoDuplicateKeys(): ' + + 'Tried to merge two objects with the same key: `%s`. This conflict ' + + 'may be due to a mixin; in particular, this may be caused by two ' + + 'getInitialState() or getDefaultProps() methods returning objects ' + + 'with clashing keys.', + key + ); + one[key] = two[key]; + } + } return one; } @@ -604,7 +605,10 @@ function createMergedResultFunction(one, two) { } else if (b == null) { return a; } - return mergeObjectsWithNoDuplicateKeys(a, b); + var c = {}; + mergeIntoWithNoDuplicateKeys(c, a); + mergeIntoWithNoDuplicateKeys(c, b); + return c; }; } diff --git a/src/core/__tests__/ReactCompositeComponent-test.js b/src/core/__tests__/ReactCompositeComponent-test.js index 1510af60113..a8e41988cb9 100644 --- a/src/core/__tests__/ReactCompositeComponent-test.js +++ b/src/core/__tests__/ReactCompositeComponent-test.js @@ -586,7 +586,7 @@ describe('ReactCompositeComponent', function() { expect(function() { instance = ReactTestUtils.renderIntoDocument(instance); }).toThrow( - 'Invariant Violation: mergeObjectsWithNoDuplicateKeys(): ' + + 'Invariant Violation: mergeIntoWithNoDuplicateKeys(): ' + 'Tried to merge two objects with the same key: `x`. This conflict ' + 'may be due to a mixin; in particular, this may be caused by two ' + 'getInitialState() or getDefaultProps() methods returning objects ' + @@ -594,6 +594,26 @@ describe('ReactCompositeComponent', function() { ); }); + it('should not mutate objects returned by getInitialState()', function() { + var Mixin = { + getInitialState: function() { + return Object.freeze({mixin: true}); + } + }; + var Component = React.createClass({ + mixins: [Mixin], + getInitialState: function() { + return Object.freeze({component: true}); + }, + render: function() { + return ; + } + }); + expect(() => { + ReactTestUtils.renderIntoDocument(); + }).not.toThrow(); + }); + it('should work with object getInitialState() return values', function() { var Component = React.createClass({ getInitialState: function() {