diff --git a/scripts/fiber/tests-passing.txt b/scripts/fiber/tests-passing.txt index 0cf8d02409..08ecd7b493 100644 --- a/scripts/fiber/tests-passing.txt +++ b/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 diff --git a/scripts/rollup/results.json b/scripts/rollup/results.json index 01bee146ee..a45b614277 100644 --- a/scripts/rollup/results.json +++ b/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 } } } \ No newline at end of file diff --git a/src/isomorphic/children/__tests__/ReactChildren-test.js b/src/isomorphic/children/__tests__/ReactChildren-test.js index a71ea8f7a7..9a2e374db7 100644 --- a/src/isomorphic/children/__tests__/ReactChildren-test.js +++ b/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 [
,
]; + } + } + + ReactTestUtils.renderIntoDocument(); + + 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 [
,
]; + } + } + + ReactTestUtils.renderIntoDocument(); + + expectDev(console.error.calls.count()).toBe(0); + }); + }); + } }); diff --git a/src/isomorphic/classic/element/ReactElementValidator.js b/src/isomorphic/classic/element/ReactElementValidator.js index 9840ba3a59..abcb4709e0 100644 --- a/src/isomorphic/classic/element/ReactElementValidator.js +++ b/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 diff --git a/src/renderers/__tests__/ReactMultiChildText-test.js b/src/renderers/__tests__/ReactMultiChildText-test.js index b9598076ba..199ed41d2e 100644 --- a/src/renderers/__tests__/ReactMultiChildText-test.js +++ b/src/renderers/__tests__/ReactMultiChildText-test.js @@ -191,10 +191,20 @@ describe('ReactMultiChildText', () => { [true,
{1.2}{''}{
}{'foo'}
, true, 1.2], [
, '1.2'], ['', 'foo',
{true}{
}{1.2}{''}
, 'foo'], ['', 'foo',
, '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', () => { diff --git a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js b/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js index 026fa91035..60ec33ae88 100644 --- a/src/renderers/dom/fiber/__tests__/ReactDOMFiber-test.js +++ b/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 [
, ]; + return [
, ]; } } @@ -141,7 +141,7 @@ describe('ReactDOMFiber', () => { class Fragment extends React.Component { render() { - return [
, ]; + return [
, ]; } } @@ -164,7 +164,7 @@ describe('ReactDOMFiber', () => { class Fragment extends React.Component { render() { - return [,
, ]; + return [,
, ]; } } @@ -263,16 +263,16 @@ describe('ReactDOMFiber', () => { render() { const {step} = this.props; return [ - , + , ReactDOM.unstable_createPortal( - , + , portalContainer1, ), - , + , ReactDOM.unstable_createPortal( [ - , - , + , + , ], portalContainer2, ), @@ -337,23 +337,23 @@ describe('ReactDOMFiber', () => { ReactDOM.render( [ -
normal[0]
, +
normal[0]
, ReactDOM.unstable_createPortal( [ -
portal1[0]
, +
portal1[0]
, ReactDOM.unstable_createPortal( -
portal2[0]
, +
portal2[0]
, portalContainer2, ), ReactDOM.unstable_createPortal( -
portal3[0]
, +
portal3[0]
, portalContainer3, ), -
portal1[1]
, +
portal1[1]
, ], portalContainer1, ), -
normal[1]
, +
normal[1]
, ], container, ); @@ -943,7 +943,7 @@ describe('ReactDOMFiber', () => { } let inst; - ReactDOM.render([ inst = n} />], container); + ReactDOM.render([ 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([, ], container); + ReactDOM.render( + [, ], + 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([
], container)).toThrow( + expect(() => ReactDOM.render([
], container)).toThrow( message, container, ); @@ -1048,7 +1051,7 @@ describe('disableNewFiberFeatures', () => { /You may have returned undefined/, ); expect(() => - ReactDOM.render([
], container)).toThrow( + ReactDOM.render([
], container)).toThrow( /You may have returned undefined/, ); }); diff --git a/src/renderers/shared/fiber/ReactChildFiber.js b/src/renderers/shared/fiber/ReactChildFiber.js index ac6eab0138..347722fed0 100644 --- a/src/renderers/shared/fiber/ReactChildFiber.js +++ b/src/renderers/shared/fiber/ReactChildFiber.js @@ -43,6 +43,43 @@ if (__DEV__) { var getComponentName = require('getComponentName'); var warning = require('fbjs/lib/warning'); var didWarnAboutMaps = false; + /** + * Warn if there's no key explicitly set on dynamic arrays of children or + * object keys are not valid. This allows us to keep track of children between + * updates. + */ + var ownerHasKeyUseWarning = {}; + + var warnForMissingKey = (child: mixed) => { + if (child === null || typeof child !== 'object') { + return; + } + if (!child._store || child._store.validated || child.key != null) { + return; + } + invariant( + typeof child._store === 'object', + 'React Component in warnForMissingKey should have a _store', + ); + child._store.validated = true; + + var currentComponentErrorInfo = 'Each child in an array or iterator should have a unique ' + + '"key" prop. See https://fb.me/react-warning-keys for ' + + 'more information.' + + (getCurrentFiberStackAddendum(child) || ''); + if (ownerHasKeyUseWarning[currentComponentErrorInfo]) { + return; + } + ownerHasKeyUseWarning[currentComponentErrorInfo] = true; + + warning( + false, + 'Each child in an array or iterator should have a unique ' + + '"key" prop. See https://fb.me/react-warning-keys for ' + + 'more information.%s', + getCurrentFiberStackAddendum(child), + ); + }; } const { @@ -592,7 +629,10 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { return null; } - function warnOnDuplicateKey( + /** + * Warns if there is a duplicate or missing key + */ + function warnOnInvalidKey( child: mixed, knownKeys: Set | null, ): Set | null { @@ -604,6 +644,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { case REACT_ELEMENT_TYPE: case REACT_COROUTINE_TYPE: case REACT_PORTAL_TYPE: + warnForMissingKey(child); const key = child.key; if (typeof key !== 'string') { break; @@ -663,7 +704,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { let knownKeys = null; for (let i = 0; i < newChildren.length; i++) { const child = newChildren[i]; - knownKeys = warnOnDuplicateKey(child, knownKeys); + knownKeys = warnOnInvalidKey(child, knownKeys); } } @@ -842,7 +883,7 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) { let step = newChildren.next(); for (; !step.done; step = newChildren.next()) { const child = step.value; - knownKeys = warnOnDuplicateKey(child, knownKeys); + knownKeys = warnOnInvalidKey(child, knownKeys); } } } diff --git a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js b/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js index 88d35fff2f..7e533ce3f1 100644 --- a/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactCoroutine-test.js @@ -57,13 +57,13 @@ describe('ReactCoroutine', () => { function Indirection() { ops.push('Indirection'); - return [, ]; + return [, ]; } function HandleYields(props, yields) { ops.push('HandleYields'); - return yields.map(y => ( - + return yields.map((y, i) => ( + )); } @@ -117,12 +117,12 @@ describe('ReactCoroutine', () => { } function Indirection() { - return [, ]; + return [, ]; } function HandleYields(props, yields) { - return yields.map(y => ( - + return yields.map((y, i) => ( + )); } @@ -176,7 +176,9 @@ describe('ReactCoroutine', () => { function HandleYields(props, yields) { ops.push('HandleYields'); - return yields.map(ContinuationComponent => ); + return yields.map((ContinuationComponent, i) => ( + + )); } class Parent extends React.Component { @@ -223,8 +225,12 @@ describe('ReactCoroutine', () => { function App(props) { return ReactCoroutine.createCoroutine( - [, , ], - (p, yields) => yields.map(y => ), + [ + , + , + , + ], + (p, yields) => yields.map((y, i) => ), {}, ); } diff --git a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js index b66151f38e..93e874edfc 100644 --- a/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js +++ b/src/renderers/shared/fiber/__tests__/ReactIncremental-test.js @@ -51,7 +51,7 @@ describe('ReactIncremental', () => { var fooCalled = false; function Foo() { fooCalled = true; - return [, ]; + return [, ]; } ReactNoop.render(, () => renderCallbackCalled = true); @@ -384,8 +384,8 @@ describe('ReactIncremental', () => { } render() { return [ - , -