Warn for keys in fragments - third approach

scripts/fiber/tests-passing.txt

@@ -76,6 +76,8 @@ src/isomorphic/children/__tests__/ReactChildren-test.js
 * should flatten children to an array
 * should throw on object
 * should throw on regex
+* warns for keys for arrays of elements in a fragment
+* does not warn when there are keys on  elements in a fragment
 
 src/isomorphic/children/__tests__/onlyChild-test.js
 * should fail when passed two children

scripts/rollup/results.json

@@ -1,17 +1,17 @@
 {
-  "branch": "master",
+  "branch": "warnForKeysInFragmentsRefactorThirdApproach",
   "bundleSizes": {
     "react.development.js (UMD_DEV)": {
-      "size": 121454,
-      "gzip": 30515
+      "size": 121387,
+      "gzip": 30491
     },
     "react.production.min.js (UMD_PROD)": {
       "size": 15685,
       "gzip": 5765
     },
     "react-dom.development.js (UMD_DEV)": {
-      "size": 583190,
-      "gzip": 134534
+      "size": 584434,
+      "gzip": 134865
     },
     "react-dom.production.min.js (UMD_PROD)": {
       "size": 120740,
@@ -26,24 +26,24 @@
       "gzip": 33273
     },
     "react-art.development.js (UMD_DEV)": {
-      "size": 342608,
-      "gzip": 76782
+      "size": 343852,
+      "gzip": 77090
     },
     "react-art.production.min.js (UMD_PROD)": {
       "size": 95013,
       "gzip": 28991
     },
     "react.development.js (NODE_DEV)": {
-      "size": 70266,
-      "gzip": 17594
+      "size": 70199,
+      "gzip": 17572
     },
     "react.production.min.js (NODE_PROD)": {
       "size": 9226,
       "gzip": 3628
     },
     "React-dev.js (FB_DEV)": {
-      "size": 72123,
-      "gzip": 18231
+      "size": 72056,
+      "gzip": 18209
     },
     "React-prod.js (FB_PROD)": {
       "size": 36643,
@@ -58,20 +58,20 @@
       "gzip": 84675
     },
     "react-dom.development.js (NODE_DEV)": {
-      "size": 542188,
-      "gzip": 125158
+      "size": 543430,
+      "gzip": 125486
     },
     "react-dom.production.min.js (NODE_PROD)": {
       "size": 116925,
       "gzip": 36732
     },
     "ReactDOMFiber-dev.js (FB_DEV)": {
-      "size": 797235,
-      "gzip": 184122
+      "size": 798477,
+      "gzip": 184445
     },
     "ReactDOMFiber-prod.js (FB_PROD)": {
-      "size": 407613,
-      "gzip": 93586
+      "size": 407677,
+      "gzip": 93615
     },
     "react-dom-server.development.js (NODE_DEV)": {
       "size": 445589,
@@ -98,20 +98,20 @@
       "gzip": 22993
     },
     "react-art.development.js (NODE_DEV)": {
-      "size": 265052,
-      "gzip": 56927
+      "size": 266294,
+      "gzip": 57234
     },
     "react-art.production.min.js (NODE_PROD)": {
       "size": 56628,
       "gzip": 17152
     },
     "ReactARTFiber-dev.js (FB_DEV)": {
-      "size": 264230,
-      "gzip": 56736
+      "size": 265472,
+      "gzip": 57048
     },
     "ReactARTFiber-prod.js (FB_PROD)": {
-      "size": 205336,
-      "gzip": 43154
+      "size": 205400,
+      "gzip": 43183
     },
     "ReactNativeStack.js (RN)": {
       "size": 233993,
@@ -122,20 +122,20 @@
       "gzip": 84001
     },
     "ReactTestRendererFiber-dev.js (FB_DEV)": {
-      "size": 262139,
-      "gzip": 55704
+      "size": 263381,
+      "gzip": 56013
     },
     "ReactTestRendererStack-dev.js (FB_DEV)": {
       "size": 151521,
       "gzip": 34765
     },
     "react-noop-renderer.development.js (NODE_DEV)": {
-      "size": 254136,
-      "gzip": 53682
+      "size": 255378,
+      "gzip": 53988
     },
     "react-test-renderer.development.js (NODE_DEV)": {
-      "size": 262970,
-      "gzip": 55891
+      "size": 264212,
+      "gzip": 56201
     }
   }
 }

src/isomorphic/children/__tests__/ReactChildren-test.js

@@ -11,12 +11,21 @@
 
 'use strict';
 
+const ReactDOMFeatureFlags = require('ReactDOMFeatureFlags');
+
 describe('ReactChildren', () => {
   var React;
+  var ReactTestUtils;
+  var ReactFeatureFlags;
+
+  function normalizeCodeLocInfo(str) {
+    return str && str.replace(/at .+?:\d+/g, 'at **');
+  }
 
   beforeEach(() => {
     jest.resetModules();
     React = require('react');
+    ReactTestUtils = require('ReactTestUtils');
   });
 
   it('should support identity for simple', () => {
@@ -850,4 +859,45 @@ describe('ReactChildren', () => {
         'to render a collection of children, use an array instead.',
     );
   });
+
+  if (ReactDOMFeatureFlags.useFiber) {
+    describe('with fragments enabled', () => {
+      beforeEach(() => {
+        ReactFeatureFlags = require('ReactFeatureFlags');
+        ReactFeatureFlags.disableNewFiberFeatures = false;
+      });
+
+      it('warns for keys for arrays of elements in a fragment', () => {
+        spyOn(console, 'error');
+        class ComponentReturningArray extends React.Component {
+          render() {
+            return [<div />, <div />];
+          }
+        }
+
+        ReactTestUtils.renderIntoDocument(<ComponentReturningArray />);
+
+        expectDev(console.error.calls.count()).toBe(1);
+        expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
+          'Warning: ' +
+            'Each child in an array or iterator should have a unique "key" prop.' +
+            ' See https://fb.me/react-warning-keys for more information.' +
+            '\n    in ComponentReturningArray (at **)',
+        );
+      });
+
+      it('does not warn when there are keys on  elements in a fragment', () => {
+        spyOn(console, 'error');
+        class ComponentReturningArray extends React.Component {
+          render() {
+            return [<div key="foo" />, <div key="bar" />];
+          }
+        }
+
+        ReactTestUtils.renderIntoDocument(<ComponentReturningArray />);
+
+        expectDev(console.error.calls.count()).toBe(0);
+      });
+    });
+  }
 });

src/isomorphic/classic/element/ReactElementValidator.js

@@ -96,14 +96,11 @@ function validateExplicitKey(element, parentType) {
   }
   element._store.validated = true;
 
-  var memoizer = ownerHasKeyUseWarning.uniqueKey ||
-    (ownerHasKeyUseWarning.uniqueKey = {});
-
   var currentComponentErrorInfo = getCurrentComponentErrorInfo(parentType);
-  if (memoizer[currentComponentErrorInfo]) {
+  if (ownerHasKeyUseWarning[currentComponentErrorInfo]) {
     return;
   }
-  memoizer[currentComponentErrorInfo] = true;
+  ownerHasKeyUseWarning[currentComponentErrorInfo] = true;
 
   // Usually the current owner is the offender, but if it accepts children as a
   // property, it may be the creator of the child that's responsible for

src/renderers/__tests__/ReactMultiChildText-test.js

@@ -191,10 +191,20 @@ describe('ReactMultiChildText', () => {
       [true, <div>{1.2}{''}{<div />}{'foo'}</div>, true, 1.2], [<div />, '1.2'],
       ['', 'foo', <div>{true}{<div />}{1.2}{''}</div>, 'foo'], ['', 'foo', <div />, 'foo'],
     ]);
-    expectDev(console.error.calls.count()).toBe(1);
-    expectDev(console.error.calls.argsFor(0)[0]).toContain(
-      'Warning: Each child in an array or iterator should have a unique "key" prop.',
-    );
+    if (ReactDOMFeatureFlags.useFiber) {
+      expectDev(console.error.calls.count()).toBe(2);
+      expectDev(console.error.calls.argsFor(0)[0]).toContain(
+        'Warning: Each child in an array or iterator should have a unique "key" prop.',
+      );
+      expectDev(console.error.calls.argsFor(1)[0]).toContain(
+        'Warning: Each child in an array or iterator should have a unique "key" prop.',
+      );
+    } else {
+      expectDev(console.error.calls.count()).toBe(1);
+      expectDev(console.error.calls.argsFor(0)[0]).toContain(
+        'Warning: Each child in an array or iterator should have a unique "key" prop.',
+      );
+    }
   });
 
   it('should throw if rendering both HTML and children', () => {

src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js

@@ -118,7 +118,7 @@ describe('ReactDOMFiber', () => {
     it('finds the first child when a component returns a fragment', () => {
       class Fragment extends React.Component {
         render() {
-          return [<div />, <span />];
+          return [<div key="a" />, <span key="b" />];
         }
       }
 
@@ -141,7 +141,7 @@ describe('ReactDOMFiber', () => {
 
       class Fragment extends React.Component {
         render() {
-          return [<Wrapper><div /></Wrapper>, <span />];
+          return [<Wrapper key="a"><div /></Wrapper>, <span key="b" />];
         }
       }
 
@@ -164,7 +164,7 @@ describe('ReactDOMFiber', () => {
 
       class Fragment extends React.Component {
         render() {
-          return [<NullComponent />, <div />, <span />];
+          return [<NullComponent key="a" />, <div key="b" />, <span key="c" />];
         }
       }
 
@@ -263,16 +263,16 @@ describe('ReactDOMFiber', () => {
         render() {
           const {step} = this.props;
           return [
-            <Child name={`normal[0]:${step}`} />,
+            <Child key="a" name={`normal[0]:${step}`} />,
             ReactDOM.unstable_createPortal(
-              <Child name={`portal1[0]:${step}`} />,
+              <Child key="b" name={`portal1[0]:${step}`} />,
               portalContainer1,
             ),
-            <Child name={`normal[1]:${step}`} />,
+            <Child key="c" name={`normal[1]:${step}`} />,
             ReactDOM.unstable_createPortal(
               [
-                <Child name={`portal2[0]:${step}`} />,
-                <Child name={`portal2[1]:${step}`} />,
+                <Child key="d" name={`portal2[0]:${step}`} />,
+                <Child key="e" name={`portal2[1]:${step}`} />,
               ],
               portalContainer2,
             ),
@@ -337,23 +337,23 @@ describe('ReactDOMFiber', () => {
 
       ReactDOM.render(
         [
-          <div>normal[0]</div>,
+          <div key="a">normal[0]</div>,
           ReactDOM.unstable_createPortal(
             [
-              <div>portal1[0]</div>,
+              <div key="b">portal1[0]</div>,
               ReactDOM.unstable_createPortal(
-                <div>portal2[0]</div>,
+                <div key="c">portal2[0]</div>,
                 portalContainer2,
               ),
               ReactDOM.unstable_createPortal(
-                <div>portal3[0]</div>,
+                <div key="d">portal3[0]</div>,
                 portalContainer3,
               ),
-              <div>portal1[1]</div>,
+              <div key="e">portal1[1]</div>,
             ],
             portalContainer1,
           ),
-          <div>normal[1]</div>,
+          <div key="f">normal[1]</div>,
         ],
         container,
       );
@@ -943,7 +943,7 @@ describe('ReactDOMFiber', () => {
       }
 
       let inst;
-      ReactDOM.render([<Example ref={n => inst = n} />], container);
+      ReactDOM.render([<Example key="a" ref={n => inst = n} />], container);
       const node = container.firstChild;
       expect(node.tagName).toEqual('DIV');
 
@@ -981,7 +981,10 @@ describe('ReactDOMFiber', () => {
       // click handler during render to simulate a click during an aborted
       // render. I use this hack because at current time we don't have a way to
       // test aborted ReactDOM renders.
-      ReactDOM.render([<Example forceA={true} />, <Click />], container);
+      ReactDOM.render(
+        [<Example key="a" forceA={true} />, <Click key="b" />],
+        container,
+      );
 
       // Because the new click handler has not yet committed, we should still
       // invoke B.
@@ -1030,7 +1033,7 @@ describe('disableNewFiberFeatures', () => {
     expect(() => ReactDOM.render(false, container)).toThrow(message, container);
     expect(() => ReactDOM.render('Hi', container)).toThrow(message, container);
     expect(() => ReactDOM.render(999, container)).toThrow(message, container);
-    expect(() => ReactDOM.render([<div />], container)).toThrow(
+    expect(() => ReactDOM.render([<div key="a" />], container)).toThrow(
       message,
       container,
     );
@@ -1048,7 +1051,7 @@ describe('disableNewFiberFeatures', () => {
       /You may have returned undefined/,
     );
     expect(() =>
-      ReactDOM.render(<Render>[<div />]</Render>, container)).toThrow(
+      ReactDOM.render(<Render>[<div key="a" />]</Render>, container)).toThrow(
       /You may have returned undefined/,
     );
   });