From 3e48422fdc2feaf741d768a6071a87adf2fc107b Mon Sep 17 00:00:00 2001 From: Flarnie Marchan Date: Wed, 19 Apr 2017 08:05:04 -0700 Subject: [PATCH] Warn for keys in fragments - third approach (#9445) * Fix tests to pass when we warn for missing keys in fragments In most cases we just needed to add the 'key' prop. This ignores the tests which are already failing on master when running with ` REACT_DOM_JEST_USE_FIBER=1` - there are 8. All tests should now pass with `npm run test`, and the 8 which fail when running `REACT_DOM_JEST_USE_FIBER=1 npm run test` are the same 8 which are failing on master. * Added missing key warning for children in array fragments After trying twice to reuse the code between the ReactChildFiber and ReactElementValidator, I am thinking that it's simpler to just have some duplication of code. The parts that are shared are interleaved with parts which cannot be shared, either because of singleton modules that must be required differently in 'isomorphic' and the 'renderers', or the fact that 'warning' requires a hard coded string. Test Plan: - Added test to ReactChildren-test - Manually tested via fixture that was not committed. * commit updated "scripts/rollup/results.json" * Make 'ReactChildren-test' more specific, and remove unneeded nesting Based on helpful tips from @spicyj and @aweary's review - Made the unit test for the warning on missing keys more specific - Removed unneeded nesting in the code which generates missing key warning - Change test syntax to use JSX to be more consistent Also fixes flow warning. * Commit update of scripts/rollup/results.json * run "scripts/fiber/record-tests" --- scripts/fiber/tests-passing.txt | 2 + scripts/rollup/results.json | 58 +++++++++---------- .../children/__tests__/ReactChildren-test.js | 50 ++++++++++++++++ .../classic/element/ReactElementValidator.js | 7 +-- .../__tests__/ReactMultiChildText-test.js | 18 ++++-- .../dom/fiber/__tests__/ReactDOMFiber-test.js | 39 +++++++------ src/renderers/shared/fiber/ReactChildFiber.js | 47 ++++++++++++++- .../fiber/__tests__/ReactCoroutine-test.js | 24 +++++--- .../fiber/__tests__/ReactIncremental-test.js | 22 +++---- .../__tests__/ReactIncrementalPerf-test.js | 6 +- .../ReactIncrementalReflection-test.js | 2 +- .../ReactIncrementalSideEffects-test.js | 5 +- .../__tests__/ReactTopLevelFragment-test.js | 8 +-- .../__tests__/ReactTestRenderer-test.js | 6 +- 14 files changed, 203 insertions(+), 91 deletions(-) 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 [ - , -