From 49238b944046abf086ba80ab5e984ce8a8175a1d Mon Sep 17 00:00:00 2001 From: Jim Date: Wed, 15 Jun 2016 22:22:31 -0700 Subject: [PATCH] Warn if people mutate children. (#7001) --- .../classic/element/ReactElement.js | 8 +++ src/renderers/dom/shared/ReactDOMComponent.js | 14 ++++ src/renderers/shared/ReactDebugTool.js | 10 +++ .../ReactChildrenMutationWarningDevtool.js | 66 +++++++++++++++++++ .../reconciler/ReactCompositeComponent.js | 14 ++++ .../__tests__/ReactComponent-test.js | 30 +++++++++ 6 files changed, 142 insertions(+) create mode 100644 src/renderers/shared/devtools/ReactChildrenMutationWarningDevtool.js diff --git a/src/isomorphic/classic/element/ReactElement.js b/src/isomorphic/classic/element/ReactElement.js index 86ca0c9aa5332..710bcaa532546 100644 --- a/src/isomorphic/classic/element/ReactElement.js +++ b/src/isomorphic/classic/element/ReactElement.js @@ -97,6 +97,7 @@ var ReactElement = function(type, key, ref, self, source, owner, props) { // This can be replaced with a WeakMap once they are implemented in // commonly used development environments. element._store = {}; + var shadowChildren = Array.isArray(props.children) ? props.children.slice(0) : props.children; // To make comparing ReactElements easier for testing purposes, we make // the validation flag non-enumerable (where possible, which should @@ -116,6 +117,12 @@ var ReactElement = function(type, key, ref, self, source, owner, props) { writable: false, value: self, }); + Object.defineProperty(element, '_shadowChildren', { + configurable: false, + enumerable: false, + writable: false, + value: shadowChildren, + }); // Two elements created in two different places should be considered // equal for testing purposes and therefore we hide it from enumeration. Object.defineProperty(element, '_source', { @@ -127,6 +134,7 @@ var ReactElement = function(type, key, ref, self, source, owner, props) { } else { element._store.validated = false; element._self = self; + element._shadowChildren = shadowChildren; element._source = source; } if (Object.freeze) { diff --git a/src/renderers/dom/shared/ReactDOMComponent.js b/src/renderers/dom/shared/ReactDOMComponent.js index e0ae87148e0e6..9494691605917 100644 --- a/src/renderers/dom/shared/ReactDOMComponent.js +++ b/src/renderers/dom/shared/ReactDOMComponent.js @@ -713,6 +713,13 @@ ReactDOMComponent.Mixin = { break; } + if (__DEV__) { + if (this._debugID) { + var callback = () => ReactInstrumentation.debugTool.onComponentHasMounted(this._debugID); + transaction.getReactMountReady().enqueue(callback, this); + } + } + return mountImage; }, @@ -933,6 +940,13 @@ ReactDOMComponent.Mixin = { // reconciliation transaction.getReactMountReady().enqueue(postUpdateSelectWrapper, this); } + + if (__DEV__) { + if (this._debugID) { + var callback = () => ReactInstrumentation.debugTool.onComponentHasUpdated(this._debugID); + transaction.getReactMountReady().enqueue(callback, this); + } + } }, /** diff --git a/src/renderers/shared/ReactDebugTool.js b/src/renderers/shared/ReactDebugTool.js index 6deb84bc85be6..f386cd21e69df 100644 --- a/src/renderers/shared/ReactDebugTool.js +++ b/src/renderers/shared/ReactDebugTool.js @@ -258,6 +258,14 @@ var ReactDebugTool = { checkDebugID(debugID); emitEvent('onHostOperation', debugID, type, payload); }, + onComponentHasMounted(debugID) { + checkDebugID(debugID); + emitEvent('onComponentHasMounted', debugID); + }, + onComponentHasUpdated(debugID) { + checkDebugID(debugID); + emitEvent('onComponentHasUpdated', debugID); + }, onSetState() { emitEvent('onSetState'); }, @@ -314,9 +322,11 @@ if (__DEV__) { var ReactInvalidSetStateWarningDevTool = require('ReactInvalidSetStateWarningDevTool'); var ReactHostOperationHistoryDevtool = require('ReactHostOperationHistoryDevtool'); var ReactComponentTreeDevtool = require('ReactComponentTreeDevtool'); + var ReactChildrenMutationWarningDevtool = require('ReactChildrenMutationWarningDevtool'); ReactDebugTool.addDevtool(ReactInvalidSetStateWarningDevTool); ReactDebugTool.addDevtool(ReactComponentTreeDevtool); ReactDebugTool.addDevtool(ReactHostOperationHistoryDevtool); + ReactDebugTool.addDevtool(ReactChildrenMutationWarningDevtool); var url = (ExecutionEnvironment.canUseDOM && window.location.href) || ''; if ((/[?&]react_perf\b/).test(url)) { ReactDebugTool.beginProfiling(); diff --git a/src/renderers/shared/devtools/ReactChildrenMutationWarningDevtool.js b/src/renderers/shared/devtools/ReactChildrenMutationWarningDevtool.js new file mode 100644 index 0000000000000..9cde836a278b0 --- /dev/null +++ b/src/renderers/shared/devtools/ReactChildrenMutationWarningDevtool.js @@ -0,0 +1,66 @@ +/** + * Copyright 2013-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + * + * @providesModule ReactChildrenMutationWarningDevtool + */ + +'use strict'; + +var ReactComponentTreeDevtool = require('ReactComponentTreeDevtool'); + +var warning = require('warning'); + +var elements = {}; + +function handleElement(debugID, element) { + if (element == null) { + return; + } + if (element._shadowChildren === undefined) { + return; + } + if (element._shadowChildren === element.props.children) { + return; + } + var isMutated = false; + if (Array.isArray(element._shadowChildren)) { + if (element._shadowChildren.length === element.props.children.length) { + for (var i = 0; i < element._shadowChildren.length; i++) { + if (element._shadowChildren[i] !== element.props.children[i]) { + isMutated = true; + } + } + } else { + isMutated = true; + } + } + warning( + Array.isArray(element._shadowChildren) && !isMutated, + 'Component\'s children should not be mutated.%s', + ReactComponentTreeDevtool.getStackAddendumByID(debugID), + ); +} + +var ReactDOMUnknownPropertyDevtool = { + onBeforeMountComponent(debugID, element) { + elements[debugID] = element; + }, + onBeforeUpdateComponent(debugID, element) { + elements[debugID] = element; + }, + onComponentHasMounted(debugID) { + handleElement(debugID, elements[debugID]); + delete elements[debugID]; + }, + onComponentHasUpdated(debugID) { + handleElement(debugID, elements[debugID]); + delete elements[debugID]; + }, +}; + +module.exports = ReactDOMUnknownPropertyDevtool; diff --git a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js index ee158ec600178..d12995062a724 100644 --- a/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js +++ b/src/renderers/shared/stack/reconciler/ReactCompositeComponent.js @@ -342,6 +342,13 @@ var ReactCompositeComponentMixin = { } } + if (__DEV__) { + if (this._debugID) { + var callback = (component) => ReactInstrumentation.debugTool.onComponentHasMounted(this._debugID); + transaction.getReactMountReady().enqueue(callback, this); + } + } + return markup; }, @@ -952,6 +959,13 @@ var ReactCompositeComponentMixin = { ); } } + + if (__DEV__) { + if (this._debugID) { + var callback = () => ReactInstrumentation.debugTool.onComponentHasUpdated(this._debugID); + transaction.getReactMountReady().enqueue(callback, this); + } + } }, /** diff --git a/src/renderers/shared/stack/reconciler/__tests__/ReactComponent-test.js b/src/renderers/shared/stack/reconciler/__tests__/ReactComponent-test.js index cd0c13d6e5f92..8039e88e98fd0 100644 --- a/src/renderers/shared/stack/reconciler/__tests__/ReactComponent-test.js +++ b/src/renderers/shared/stack/reconciler/__tests__/ReactComponent-test.js @@ -15,6 +15,10 @@ var React; var ReactDOM; var ReactTestUtils; +function normalizeCodeLocInfo(str) { + return str.replace(/\(at .+?:\d+\)/g, '(at **)'); +} + describe('ReactComponent', function() { beforeEach(function() { React = require('React'); @@ -45,6 +49,32 @@ describe('ReactComponent', function() { }).toThrow(); }); + it('should warn when children are mutated before render', function() { + spyOn(console, 'error'); + var children = [, , ]; + var element =
{children}
; + children[1] =

; // Mutation is illegal + ReactTestUtils.renderIntoDocument(element); + expect(console.error.calls.count()).toBe(1); + expect(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: Component\'s children should not be mutated.\n in div (at **)' + ); + }); + + it('should warn when children are mutated', function() { + spyOn(console, 'error'); + var children = [, , ]; + function Wrapper(props) { + props.children[1] =

; // Mutation is illegal + return

{props.children}
; + } + ReactTestUtils.renderIntoDocument({children}); + expect(console.error.calls.count()).toBe(1); + expect(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe( + 'Warning: Component\'s children should not be mutated.\n in Wrapper (at **)' + ); + }); + it('should support refs on owned components', function() { var innerObj = {}; var outerObj = {};