From 086636747f26b577b4a4577a0888118310ee91b3 Mon Sep 17 00:00:00 2001 From: Ben Alpert Date: Mon, 27 Apr 2015 14:16:14 -0700 Subject: [PATCH] Add key warning to nested collections Also when reusing elements in multiple contexts -- before we were mutating each element to indicate its validity; now we mutate the array containing it (which we create, in the case of rest-arg children). Fixes #2496. Fixes #3348. --- src/addons/__tests__/ReactFragment-test.js | 6 +- src/classic/element/ReactElement.js | 34 ++++----- src/classic/element/ReactElementValidator.js | 16 +++- .../__tests__/ReactElementValidator-test.js | 75 +++++++++++++++++++ 4 files changed, 105 insertions(+), 26 deletions(-) diff --git a/src/addons/__tests__/ReactFragment-test.js b/src/addons/__tests__/ReactFragment-test.js index d53e6020de..6b789e016a 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/classic/element/ReactElement.js b/src/classic/element/ReactElement.js index 4760c6311d..53e9f5f2da 100644 --- a/src/classic/element/ReactElement.js +++ b/src/classic/element/ReactElement.js @@ -106,20 +106,6 @@ 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. @@ -170,6 +156,21 @@ ReactElement.createElement = function(type, config, children) { props.children = children; } else if (childrenLength > 1) { var childArray = Array(childrenLength); + + // 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; + for (var i = 0; i < childrenLength; i++) { childArray[i] = arguments[i + 2]; } @@ -216,11 +217,6 @@ 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; }; diff --git a/src/classic/element/ReactElementValidator.js b/src/classic/element/ReactElementValidator.js index 48b0f44d6d..36ba7bfd09 100644 --- a/src/classic/element/ReactElementValidator.js +++ b/src/classic/element/ReactElementValidator.js @@ -92,11 +92,9 @@ function getCurrentOwnerDisplayName() { * @param {*} parentType element's parent's type. */ function validateExplicitKey(element, parentType) { - if (element._store.validated || element.key != null) { + if (element.key != null) { return; } - element._store.validated = true; - warnAndMonitorForKeyUse( 'Each child in an array or iterator should have a unique "key" prop.', element, @@ -183,15 +181,22 @@ function warnAndMonitorForKeyUse(message, element, parentType) { */ 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); + } else { + // TODO: Warn on unkeyed arrays and suggest using createFragment + validateChildKeys(child, parentType); } } } else if (ReactElement.isValidElement(node)) { // This element was passed in a valid location. - node._store.validated = true; + return; } else if (node) { var iteratorFn = getIteratorFn(node); // Entry iterators provide implicit keys. @@ -202,6 +207,8 @@ function validateChildKeys(node, parentType) { while (!(step = iterator.next()).done) { if (ReactElement.isValidElement(step.value)) { validateExplicitKey(step.value, parentType); + } else { + validateChildKeys(step.value, parentType); } } } @@ -210,6 +217,7 @@ function validateChildKeys(node, parentType) { for (var key in fragment) { if (fragment.hasOwnProperty(key)) { validatePropertyKey(key, fragment[key], parentType); + validateChildKeys(fragment[key], parentType); } } } diff --git a/src/classic/element/__tests__/ReactElementValidator-test.js b/src/classic/element/__tests__/ReactElementValidator-test.js index a4b82e3907..01b1a1b240 100644 --- a/src/classic/element/__tests__/ReactElementValidator-test.js +++ b/src/classic/element/__tests__/ReactElementValidator-test.js @@ -120,6 +120,81 @@ 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 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('warns for keys when reusing children', function() { + spyOn(console, 'error'); + + var f = ; + var g = ; + + var children = [f, g]; + + return ( +
+
+ {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'); + + debugger; + var Wrapper = React.createClass({ + render: function() { + return ( +
+ {this.props.children} +
+
+ ); + } + }); + + ReactTestUtils.renderIntoDocument( + + + + + ); + + expect(console.error.argsForCall.length).toBe(0); + }); + it('warns for keys for iterables of elements in rest args', function() { spyOn(console, 'error'); var Component = React.createFactory(ComponentClass);