From 39402fe75d3657615aa9b4ae0be73dab2665321e Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Thu, 14 May 2015 16:33:26 -0700 Subject: [PATCH 1/2] Revert "Add key warning to nested collections" This heuristic isn't great because it relies on inspecting deep children which aren't guaranteed to be React elements. In particular, this was causing stack overflows in a component we had that used a *DOM node* as children, like `{node}`. This reverts commits: 0a3aa8493aa014668f5dd3bac30868c18397538e 64c9d9d7629600da81ceefcaa59c46e64d030874 0c58f4f6b12bd7856b73f14163db905cb13c5fc7 8cf226e44241aeafe147f6256a1351b46ac3cf91 086636747f26b577b4a4577a0888118310ee91b3 --- src/addons/__tests__/ReactFragment-test.js | 6 +-- .../classic/element/ReactElement.js | 40 +++++++------- .../classic/element/ReactElementValidator.js | 52 ++++++------------- .../__tests__/ReactElementValidator-test.js | 50 ------------------ 4 files changed, 37 insertions(+), 111 deletions(-) diff --git a/src/addons/__tests__/ReactFragment-test.js b/src/addons/__tests__/ReactFragment-test.js index 6b789e016a..d53e6020de 100644 --- a/src/addons/__tests__/ReactFragment-test.js +++ b/src/addons/__tests__/ReactFragment-test.js @@ -49,13 +49,13 @@ describe('ReactFragment', function() { z: }; var element =
{[children]}
; + expect(console.error.calls.length).toBe(0); + var container = document.createElement('div'); + React.render(element, container); expect(console.error.calls.length).toBe(1); expect(console.error.calls[0].args[0]).toContain( 'Any use of a keyed object' ); - var container = document.createElement('div'); - React.render(element, container); - expect(console.error.calls.length).toBe(1); }); it('should warn if accessing any property on a fragment', function() { diff --git a/src/isomorphic/classic/element/ReactElement.js b/src/isomorphic/classic/element/ReactElement.js index 88d7f36c3e..4760c6311d 100644 --- a/src/isomorphic/classic/element/ReactElement.js +++ b/src/isomorphic/classic/element/ReactElement.js @@ -80,21 +80,6 @@ function defineMutationMembrane(prototype) { } } -function markChildArrayValidated(childArray) { - // To make comparing ReactElements easier for testing purposes, we make the - // validation flag non-enumerable (where possible, which should include every - // environment we run tests in), so the test framework ignores it. - try { - Object.defineProperty(childArray, '_reactChildKeysValidated', { - configurable: false, - enumerable: false, - writable: true - }); - } catch (x) { - } - childArray._reactChildKeysValidated = true; -} - /** * Base constructor for all React elements. This is only used to make this * work with a dynamic instanceof check. Nothing should live on this prototype. @@ -121,6 +106,20 @@ var ReactElement = function(type, key, ref, owner, context, props) { // commonly used development environments. 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 + // include every environment we run tests in), so the test framework + // ignores it. + try { + Object.defineProperty(this._store, 'validated', { + configurable: false, + enumerable: false, + writable: true + }); + } catch (x) { + } + this._store.validated = false; + // We're not allowed to set props directly on the object so we early // return and rely on the prototype membrane to forward to the backing // store. @@ -171,9 +170,6 @@ ReactElement.createElement = function(type, config, children) { props.children = children; } else if (childrenLength > 1) { var childArray = Array(childrenLength); - if (__DEV__) { - markChildArrayValidated(childArray); - } for (var i = 0; i < childrenLength; i++) { childArray[i] = arguments[i + 2]; } @@ -220,6 +216,11 @@ ReactElement.cloneAndReplaceProps = function(oldElement, newProps) { oldElement._context, newProps ); + + if (__DEV__) { + // If the key on the original is valid, then the clone is valid + newElement._store.validated = oldElement._store.validated; + } return newElement; }; @@ -261,9 +262,6 @@ ReactElement.cloneElement = function(element, config, children) { props.children = children; } else if (childrenLength > 1) { var childArray = Array(childrenLength); - if (__DEV__) { - markChildArrayValidated(childArray); - } for (var i = 0; i < childrenLength; i++) { childArray[i] = arguments[i + 2]; } diff --git a/src/isomorphic/classic/element/ReactElementValidator.js b/src/isomorphic/classic/element/ReactElementValidator.js index 93d14798da..48b0f44d6d 100644 --- a/src/isomorphic/classic/element/ReactElementValidator.js +++ b/src/isomorphic/classic/element/ReactElementValidator.js @@ -90,19 +90,15 @@ function getCurrentOwnerDisplayName() { * @internal * @param {ReactElement} element Element that requires a key. * @param {*} parentType element's parent's type. - * @param {boolean} deep false if top-level collection, true if nested */ -function validateExplicitKey(element, parentType, deep) { - if (element.key != null) { +function validateExplicitKey(element, parentType) { + if (element._store.validated || element.key != null) { return; } + element._store.validated = true; + warnAndMonitorForKeyUse( - // We vary the message for nested key warning to allow filtering them out - // since we didn't historically warn in this case. - deep ? - 'Each child in a nested array or iterator should have ' + - 'a unique "key" prop.' : - 'Each child in an array or iterator should have a unique "key" prop.', + 'Each child in an array or iterator should have a unique "key" prop.', element, parentType ); @@ -116,17 +112,13 @@ function validateExplicitKey(element, parentType, deep) { * @param {string} name Property name of the key. * @param {ReactElement} element Component that requires a key. * @param {*} parentType element's parent's type. - * @param {boolean} deep false if top-level collection, true if nested */ -function validatePropertyKey(name, element, parentType, deep) { +function validatePropertyKey(name, element, parentType) { if (!NUMERIC_PROPERTY_REGEX.test(name)) { return; } warnAndMonitorForKeyUse( - deep ? - 'Nested child objects should have non-numeric keys ' + - 'so ordering is preserved.' : - 'Child objects should have non-numeric keys so ordering is preserved.', + 'Child objects should have non-numeric keys so ordering is preserved.', element, parentType ); @@ -188,29 +180,18 @@ function warnAndMonitorForKeyUse(message, element, parentType) { * @internal * @param {ReactNode} node Statically passed child of any type. * @param {*} parentType node's parent's type. - * @param {boolean} deep false if top-level collection, true if nested */ -function validateChildKeys(node, parentType, deep) { +function validateChildKeys(node, parentType) { if (Array.isArray(node)) { - if (node._reactChildKeysValidated) { - // All child elements were passed in a valid location. - return; - } for (var i = 0; i < node.length; i++) { var child = node[i]; if (ReactElement.isValidElement(child)) { - validateExplicitKey(child, parentType, deep); - } else { - // TODO: Warn on unkeyed arrays and suggest using createFragment - validateChildKeys(child, parentType, true); + validateExplicitKey(child, parentType); } } - } else if ( - typeof node === 'string' || typeof node === 'number' || - ReactElement.isValidElement(node) - ) { + } else if (ReactElement.isValidElement(node)) { // This element was passed in a valid location. - return; + node._store.validated = true; } else if (node) { var iteratorFn = getIteratorFn(node); // Entry iterators provide implicit keys. @@ -220,9 +201,7 @@ function validateChildKeys(node, parentType, deep) { var step; while (!(step = iterator.next()).done) { if (ReactElement.isValidElement(step.value)) { - validateExplicitKey(step.value, parentType, deep); - } else { - validateChildKeys(step.value, parentType, true); + validateExplicitKey(step.value, parentType); } } } @@ -230,8 +209,7 @@ function validateChildKeys(node, parentType, deep) { var fragment = ReactFragment.extractIfFragment(node); for (var key in fragment) { if (fragment.hasOwnProperty(key)) { - validatePropertyKey(key, fragment[key], parentType, deep); - validateChildKeys(fragment[key], parentType, true); + validatePropertyKey(key, fragment[key], parentType); } } } @@ -437,7 +415,7 @@ var ReactElementValidator = { } for (var i = 2; i < arguments.length; i++) { - validateChildKeys(arguments[i], type, false); + validateChildKeys(arguments[i], type); } validatePropTypes(element); @@ -485,7 +463,7 @@ var ReactElementValidator = { cloneElement: function(element, props, children) { var newElement = ReactElement.cloneElement.apply(this, arguments); for (var i = 2; i < arguments.length; i++) { - validateChildKeys(arguments[i], newElement.type, false); + validateChildKeys(arguments[i], newElement.type); } validatePropTypes(newElement); return newElement; diff --git a/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js b/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js index fab2a7d789..26dd2fcafa 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js @@ -120,56 +120,6 @@ describe('ReactElementValidator', function() { ); }); - it('warns for keys for nested arrays of elements', function() { - spyOn(console, 'error'); - - var divs = [ - [ -
, -
- ], -
- ]; - ReactTestUtils.renderIntoDocument(
{divs}
); - - expect(console.error.argsForCall.length).toBe(1); - expect(console.error.argsForCall[0][0]).toBe( - 'Warning: Each child in a nested array or iterator should have a ' + - 'unique "key" prop. Check the React.render call using
. See ' + - 'https://fb.me/react-warning-keys for more information.' - ); - }); - - it('warns for keys when reusing children', function() { - spyOn(console, 'error'); - - var f = ; - var g = ; - - var children = [f, g]; - - ReactTestUtils.renderIntoDocument( -
-
- {g} -
-
- {f} -
-
- {children} -
-
- ); - - expect(console.error.argsForCall.length).toBe(1); - expect(console.error.argsForCall[0][0]).toBe( - 'Warning: Each child in an array or iterator should have a unique ' + - '"key" prop. Check the React.render call using
. See ' + - 'https://fb.me/react-warning-keys for more information.' - ); - }); - it('does not warn for keys when passing children down', function() { spyOn(console, 'error'); From 69e6ab5083fbecf5d543ec798a0327b67e3b616d Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Thu, 14 May 2015 16:43:39 -0700 Subject: [PATCH 2/2] Add test for DOM node as this.props.children --- .../__tests__/ReactElementValidator-test.js | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js b/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js index 26dd2fcafa..2f8d265956 100644 --- a/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js +++ b/src/isomorphic/classic/element/__tests__/ReactElementValidator-test.js @@ -475,4 +475,21 @@ describe('ReactElementValidator', function() { expect(console.error.argsForCall.length).toBe(1); }); + it('does not warn when using DOM node as children', function() { + spyOn(console, 'error'); + var DOMContainer = React.createClass({ + render: function() { + return
; + }, + componentDidMount: function() { + React.findDOMNode(this).appendChild(this.props.children); + } + }); + + var node = document.createElement('div'); + // This shouldn't cause a stack overflow or any other problems (#3883) + ReactTestUtils.renderIntoDocument({node}); + expect(console.error.argsForCall.length).toBe(0); + }); + });