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"
This commit is contained in:
Flarnie Marchan
2017-04-19 08:05:04 -07:00
committed by GitHub
parent 9526174e30
commit 3e48422fdc
14 changed files with 203 additions and 91 deletions

View File

@@ -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

View File

@@ -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
}
}
}

View File

@@ -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);
});
});
}
});

View File

@@ -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

View File

@@ -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', () => {

View File

@@ -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/,
);
});

View File

@@ -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<string> | null,
): Set<string> | 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);
}
}
}

View File

@@ -57,13 +57,13 @@ describe('ReactCoroutine', () => {
function Indirection() {
ops.push('Indirection');
return [<Child bar={true} />, <Child bar={false} />];
return [<Child key="a" bar={true} />, <Child key="b" bar={false} />];
}
function HandleYields(props, yields) {
ops.push('HandleYields');
return yields.map(y => (
<y.continuation isSame={props.foo === y.props.bar} />
return yields.map((y, i) => (
<y.continuation key={i} isSame={props.foo === y.props.bar} />
));
}
@@ -117,12 +117,12 @@ describe('ReactCoroutine', () => {
}
function Indirection() {
return [<Child bar={true} />, <Child bar={false} />];
return [<Child key="a" bar={true} />, <Child key="b" bar={false} />];
}
function HandleYields(props, yields) {
return yields.map(y => (
<y.continuation isSame={props.foo === y.props.bar} />
return yields.map((y, i) => (
<y.continuation key={i} isSame={props.foo === y.props.bar} />
));
}
@@ -176,7 +176,9 @@ describe('ReactCoroutine', () => {
function HandleYields(props, yields) {
ops.push('HandleYields');
return yields.map(ContinuationComponent => <ContinuationComponent />);
return yields.map((ContinuationComponent, i) => (
<ContinuationComponent key={i} />
));
}
class Parent extends React.Component {
@@ -223,8 +225,12 @@ describe('ReactCoroutine', () => {
function App(props) {
return ReactCoroutine.createCoroutine(
[<Counter id="a" />, <Counter id="b" />, <Counter id="c" />],
(p, yields) => yields.map(y => <span prop={y * 100} />),
[
<Counter key="a" id="a" />,
<Counter key="b" id="b" />,
<Counter key="c" id="c" />,
],
(p, yields) => yields.map((y, i) => <span key={i} prop={y * 100} />),
{},
);
}

View File

@@ -51,7 +51,7 @@ describe('ReactIncremental', () => {
var fooCalled = false;
function Foo() {
fooCalled = true;
return [<Bar isBar={true} />, <Bar isBar={true} />];
return [<Bar key="a" isBar={true} />, <Bar key="b" isBar={true} />];
}
ReactNoop.render(<Foo />, () => renderCallbackCalled = true);
@@ -384,8 +384,8 @@ describe('ReactIncremental', () => {
}
render() {
return [
<Tester unused={this.props.unused} />,
<bbb hidden={true}>
<Tester key="a" unused={this.props.unused} />,
<bbb key="b" hidden={true}>
<ccc>
<Middle>Hi</Middle>
</ccc>
@@ -583,8 +583,8 @@ describe('ReactIncremental', () => {
// low priority. I think this would be fixed by changing
// pendingWorkPriority and progressedPriority to be the priority of
// the children only, not including the fiber itself.
<div><Child /></div>,
<Sibling />,
<div key="a"><Child /></div>,
<Sibling key="b" />,
];
}
@@ -1413,7 +1413,7 @@ describe('ReactIncremental', () => {
}
function App(props) {
return [<LifeCycle x={props.x} />, <Sibling />];
return [<LifeCycle key="a" x={props.x} />, <Sibling key="b" />];
}
ReactNoop.render(<App x={0} />);
@@ -1662,13 +1662,13 @@ describe('ReactIncremental', () => {
render() {
ops.push('Indirection ' + JSON.stringify(this.context));
return [
<ShowLocale />,
<ShowRoute />,
<ShowNeither />,
<Intl locale="ru">
<ShowLocale key="a" />,
<ShowRoute key="b" />,
<ShowNeither key="c" />,
<Intl key="d" locale="ru">
<ShowBoth />
</Intl>,
<ShowBoth />,
<ShowBoth key="e" />,
];
}
}

View File

@@ -449,12 +449,12 @@ describe('ReactDebugFiberPerf', () => {
}
function Indirection() {
return [<CoChild bar={true} />, <CoChild bar={false} />];
return [<CoChild key="a" bar={true} />, <CoChild key="b" bar={false} />];
}
function HandleYields(props, yields) {
return yields.map(y => (
<y.continuation isSame={props.foo === y.props.bar} />
return yields.map((y, i) => (
<y.continuation key={i} isSame={props.foo === y.props.bar} />
));
}

View File

@@ -169,7 +169,7 @@ describe('ReactIncrementalReflection', () => {
}
function Foo(props) {
return [<Component step={props.step} />, <Sibling />];
return [<Component key="a" step={props.step} />, <Sibling key="b" />];
}
ReactNoop.render(<Foo step={0} />);

View File

@@ -565,7 +565,10 @@ describe('ReactIncrementalSideEffects', () => {
}
render() {
ops.push('Baz');
return [<Bar idx={this.props.idx} />, <Bar idx={this.props.idx} />];
return [
<Bar key="a" idx={this.props.idx} />,
<Bar key="b" idx={this.props.idx} />,
];
}
}
function Foo(props) {

View File

@@ -77,7 +77,7 @@ describe('ReactTopLevelFragment', function() {
function Fragment({condition}) {
return condition
? <Stateful key="a" />
: [[<Stateful key="a" />, <div key="b">World</div>], <div />];
: [[<Stateful key="a" />, <div key="b">World</div>], <div key="c" />];
}
ReactNoop.render(<Fragment />);
ReactNoop.flush();
@@ -106,8 +106,8 @@ describe('ReactTopLevelFragment', function() {
function Fragment({condition}) {
return condition
? [null, <Stateful />]
: [<div>Hello</div>, <Stateful />];
? [null, <Stateful key="a" />]
: [<div key="b">Hello</div>, <Stateful key="a" />];
}
ReactNoop.render(<Fragment />);
ReactNoop.flush();
@@ -144,7 +144,7 @@ describe('ReactTopLevelFragment', function() {
function Fragment({condition}) {
return condition
? [[<div key="b">World</div>, <Stateful key="a" />]]
: [[<Stateful key="a" />, <div key="b">World</div>], <div />];
: [[<Stateful key="a" />, <div key="b">World</div>], <div key="c" />];
}
ReactNoop.render(<Fragment />);
ReactNoop.flush();

View File

@@ -676,8 +676,8 @@ describe('ReactTestRenderer', () => {
var Component = props => props.children;
var renderer = ReactTestRenderer.create([
<Component>Hi</Component>,
<Component>Bye</Component>,
<Component key="a">Hi</Component>,
<Component key="b">Bye</Component>,
]);
expect(renderer.toJSON()).toEqual(['Hi', 'Bye']);
renderer.update(<div />);
@@ -686,7 +686,7 @@ describe('ReactTestRenderer', () => {
children: null,
props: {},
});
renderer.update([<div>goodbye</div>, 'world']);
renderer.update([<div key="a">goodbye</div>, 'world']);
expect(renderer.toJSON()).toEqual([
{
type: 'div',