Skip to content

Commit

Permalink
Implement duplicate key warnings
Browse files Browse the repository at this point in the history 
We keep known keys in a set in development. There is an annoying special case where we know we'll check it again because we break out of the loop early.

One test in the tree hook regresses to the failing state because it checks that the tree hook works without a Set available, but we started using Set in this code. It is not essential and we can clean this up later when we decide how to deal with polyfills.
  • Loading branch information
@gaearon
gaearon committed Dec 14, 2016
1 parent aa92f74 commit fbde80c
Show file tree
Hide file tree
Showing 6 changed files with 116 additions and 16 deletions.
1 change: 1 addition & 0 deletions scripts/fiber/tests-failing.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ src/renderers/shared/__tests__/ReactPerf-test.js

src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
* can be retrieved by ID
* works

src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
* gets recorded during an update
Expand Down
6 changes: 0 additions & 6 deletions scripts/fiber/tests-passing-except-dev.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ src/renderers/shared/hooks/__tests__/ReactComponentTreeHook-test.js
* reports update counts
* does not report top-level wrapper as a root
* registers inlined text nodes
* works

src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
* gets recorded for host roots
Expand All @@ -133,10 +132,6 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
* gets reported when a child is inserted
* gets reported when a child is removed

src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js
* warns for duplicated keys
* warns for duplicated keys with component stack info

src/renderers/shared/shared/__tests__/ReactComponentLifeCycle-test.js
* should correctly determine if a component is mounted
* should correctly determine if a null component is mounted
Expand All @@ -148,7 +143,6 @@ src/renderers/shared/shared/__tests__/ReactCompositeComponent-test.js
* should disallow nested render calls

src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
* should warn for duplicated keys with component stack info
* should warn for using maps as children with owner info

src/renderers/shared/shared/__tests__/ReactStatelessComponent-test.js
Expand Down
5 changes: 5 additions & 0 deletions scripts/fiber/tests-passing.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1279,6 +1279,10 @@ src/renderers/shared/hooks/__tests__/ReactHostOperationHistoryHook-test.js
* gets ignored if the type has not changed
* gets ignored if new html is equal

src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js
* warns for duplicated keys
* warns for duplicated keys with component stack info

src/renderers/shared/shared/__tests__/ReactComponent-test.js
* should throw when supplying a ref outside of render method
* should warn when children are mutated during render
Expand Down Expand Up @@ -1421,6 +1425,7 @@ src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
* should replace children with different constructors
* should NOT replace children with different owners
* should replace children with different keys
* should warn for duplicated keys with component stack info
* should reorder bailed-out children

src/renderers/shared/shared/__tests__/ReactMultiChildReconcile-test.js
Expand Down
114 changes: 108 additions & 6 deletions src/renderers/shared/fiber/ReactChildFiber.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ var emptyObject = require('emptyObject');
var getIteratorFn = require('getIteratorFn');
var invariant = require('invariant');

if (__DEV__) {
var ReactComponentTreeHook = require('ReactComponentTreeHook');
var { getStackAddendumByFiber } = ReactComponentTreeHook;
var warning = require('warning');
}

const {
cloneFiber,
createFiberFromElement,
Expand Down Expand Up @@ -541,6 +547,44 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
return null;
}

function warnOnDuplicateKey(
child : mixed,
returnFiber : Fiber,
knownKeys : Set<string>
) {
if (__DEV__) {
if (child == null || typeof child !== 'object') {
return;
}
switch (child.$$typeof) {
// They have keys.
case REACT_ELEMENT_TYPE:
case REACT_COROUTINE_TYPE:
case REACT_YIELD_TYPE:
case REACT_PORTAL_TYPE:
const key = child.key;
if (typeof key !== 'string') {
return;
}
if (knownKeys.has(key)) {
warning(
false,
'Encountered two children with the same key, ' +
'`%s`. Child keys must be unique; when two children share a key, ' +
'only the first child will be used.%s',
key,
getStackAddendumByFiber(returnFiber)
);
} else {
knownKeys.add(key);
}
return;
default:
return;
}
}
}

function reconcileChildrenArray(
returnFiber : Fiber,
currentFirstChild : ?Fiber,
Expand Down Expand Up @@ -569,6 +613,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
let resultingFirstChild : ?Fiber = null;
let previousNewFiber : ?Fiber = null;

let knownKeysInDev = null;

let oldFiber = currentFirstChild;
let lastPlacedIndex = 0;
let newIdx = 0;
Expand All @@ -582,10 +628,15 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
nextOldFiber = oldFiber.sibling;
}
}
const newChild = newChildren[newIdx];
if (__DEV__) {
knownKeysInDev = knownKeysInDev || new Set();
warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
}
const newFiber = updateSlot(
returnFiber,
oldFiber,
newChildren[newIdx],
newChild,
priority
);
if (!newFiber) {
Expand All @@ -596,6 +647,18 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
if (!oldFiber) {
oldFiber = nextOldFiber;
}
if (__DEV__) {
if (
knownKeysInDev != null &&
newChild != null &&
typeof newChild.key === 'string'
) {
// Since we break out of this loop without incrementing newIdx,
// we will look at this child again in the next loop.
// Forget we checked it to avoid a false positive for duplicate key.
knownKeysInDev.delete(newChild.key);
}
}
break;
}
if (shouldTrackSideEffects) {
Expand Down Expand Up @@ -630,9 +693,14 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
// If we don't have any more existing children we can choose a fast path
// since the rest will all be insertions.
for (; newIdx < newChildren.length; newIdx++) {
const newChild = newChildren[newIdx];
if (__DEV__) {
knownKeysInDev = knownKeysInDev || new Set();
warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
}
const newFiber = createChild(
returnFiber,
newChildren[newIdx],
newChild,
priority
);
if (!newFiber) {
Expand All @@ -655,11 +723,16 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {

// Keep scanning and use the map to restore deleted items as moves.
for (; newIdx < newChildren.length; newIdx++) {
const newChild = newChildren[newIdx];
if (__DEV__) {
knownKeysInDev = knownKeysInDev || new Set();
warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
}
const newFiber = updateFromMap(
existingChildren,
returnFiber,
newIdx,
newChildren[newIdx],
newChild,
priority
);
if (newFiber) {
Expand Down Expand Up @@ -705,6 +778,8 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
let resultingFirstChild : ?Fiber = null;
let previousNewFiber : ?Fiber = null;

let knownKeysInDev = null;

let oldFiber = currentFirstChild;
let lastPlacedIndex = 0;
let newIdx = 0;
Expand All @@ -720,10 +795,15 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
nextOldFiber = oldFiber.sibling;
}
}
const newChild = step.value;
if (__DEV__) {
knownKeysInDev = knownKeysInDev || new Set();
warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
}
const newFiber = updateSlot(
returnFiber,
oldFiber,
step.value,
newChild,
priority
);
if (!newFiber) {
Expand All @@ -734,6 +814,18 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
if (!oldFiber) {
oldFiber = nextOldFiber;
}
if (__DEV__) {
if (
knownKeysInDev != null &&
newChild != null &&
typeof newChild.key === 'string'
) {
// Since we break out of this loop without incrementing newIdx,
// we will look at this child again in the next loop.
// Forget we checked it to avoid a false positive for duplicate key.
knownKeysInDev.delete(newChild.key);
}
}
break;
}
if (shouldTrackSideEffects) {
Expand Down Expand Up @@ -768,9 +860,14 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {
// If we don't have any more existing children we can choose a fast path
// since the rest will all be insertions.
for (; !step.done; newIdx++, step = newChildren.next()) {
const newChild = step.value;
if (__DEV__) {
knownKeysInDev = knownKeysInDev || new Set();
warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
}
const newFiber = createChild(
returnFiber,
step.value,
newChild,
priority
);
if (!newFiber) {
Expand All @@ -793,11 +890,16 @@ function ChildReconciler(shouldClone, shouldTrackSideEffects) {

// Keep scanning and use the map to restore deleted items as moves.
for (; !step.done; newIdx++, step = newChildren.next()) {
const newChild = step.value;
if (__DEV__) {
knownKeysInDev = knownKeysInDev || new Set();
warnOnDuplicateKey(newChild, returnFiber, knownKeysInDev);
}
const newFiber = updateFromMap(
existingChildren,
returnFiber,
newIdx,
step.value,
newChild,
priority
);
if (newFiber) {
Expand Down
3 changes: 1 addition & 2 deletions src/renderers/shared/shared/__tests__/ReactChildReconciler-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,7 @@ describe('ReactChildReconciler', () => {
ReactTestUtils.renderIntoDocument(<GrandParent />);

expectDev(console.error.calls.count()).toBe(1);
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: flattenChildren(...): ' +
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toContain(
'Encountered two children with the same key, `1`. ' +
'Child keys must be unique; when two children share a key, ' +
'only the first child will be used.\n' +
Expand Down
3 changes: 1 addition & 2 deletions src/renderers/shared/shared/__tests__/ReactMultiChild-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -186,8 +186,7 @@ describe('ReactMultiChild', () => {
);

expectDev(console.error.calls.count()).toBe(1);
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toBe(
'Warning: flattenChildren(...): ' +
expectDev(normalizeCodeLocInfo(console.error.calls.argsFor(0)[0])).toContain(
'Encountered two children with the same key, `1`. ' +
'Child keys must be unique; when two children share a key, ' +
'only the first child will be used.\n' +
Expand Down

0 comments on commit fbde80c

Please sign in to comment.