Skip to content

Commit

Permalink
[Fiber] Track the Real Fiber for Key Warnings (#29791)
Browse files Browse the repository at this point in the history 
This refactors key warning to happen inline after we've matched a Fiber.
I didn't want to do that originally because it was riskier. But it turns
out to be straightforward enough.

This lets us use that Fiber as the source of the warning which matters
to DevTools because then DevTools can associate it with the right
component after it mounts.

We can also associate the duplicate key warning with this Fiber. That
way we'll get the callsite with the duplicate key on the stack and can
associate this warning with the child that had the duplicate.

I kept the forked DevTools tests because the warning now is counted on
the Child instead of the Parent (18 behavior).

However, this won't be released in 19.0.0 so I only test this in
whatever the next version is.

Doesn't seem worth it to have a test for just the 19.0.0 behavior.
  • Loading branch information
@sebmarkbage
sebmarkbage committed Jun 7, 2024
1 parent 0a5e0b0 commit a0a435d
Show file tree
Hide file tree
Showing 5 changed files with 104 additions and 72 deletions.
11 changes: 5 additions & 6 deletions packages/react-devtools-shared/src/__tests__/store-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1916,11 +1916,9 @@ describe('Store', () => {
});

// In React 19, JSX warnings were moved into the renderer - https://github.com/facebook/react/pull/29088
// When the error is emitted, the source fiber of this error is not yet mounted
// So DevTools can't connect the error and the fiber
// TODO(hoxyq): update RDT to keep track of such fibers
// @reactVersion >= 19.0
it('from react get counted [React >= 19]', () => {
// The warning is moved to the Child instead of the Parent.
// @reactVersion >= 19.0.1
it('from react get counted [React >= 19.0.1]', () => {
function Example() {
return [<Child />];
}
Expand All @@ -1936,9 +1934,10 @@ describe('Store', () => {
);

expect(store).toMatchInlineSnapshot(`
✕ 1, ⚠ 0
[root]
▾ <Example>
<Child>
<Child> ✕
`);
});

Expand Down
2 changes: 2 additions & 0 deletions packages/react-dom/src/__tests__/ReactChildReconciler-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ describe('ReactChildReconciler', () => {
'duplicated and/or omitted — the behavior is unsupported and ' +
'could change in a future version.\n' +
' in div (at **)\n' +
(gate(flags => flags.enableOwnerStacks) ? '' : ' in div (at **)\n') +
' in Component (at **)\n' +
(gate(flags => flags.enableOwnerStacks)
? ''
Expand Down Expand Up @@ -190,6 +191,7 @@ describe('ReactChildReconciler', () => {
'duplicated and/or omitted — the behavior is unsupported and ' +
'could change in a future version.\n' +
' in div (at **)\n' +
(gate(flags => flags.enableOwnerStacks) ? '' : ' in div (at **)\n') +
' in Component (at **)\n' +
(gate(flags => flags.enableOwnerStacks)
? ''
Expand Down
18 changes: 10 additions & 8 deletions packages/react-dom/src/__tests__/ReactMultiChild-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -227,12 +227,13 @@ describe('ReactMultiChild', () => {
'across updates. Non-unique keys may cause children to be ' +
'duplicated and/or omitted — the behavior is unsupported and ' +
'could change in a future version.\n' +
' in div (at **)\n' +
' in WrapperComponent (at **)\n' +
' in div (at **)' +
(gate(flags => flags.enableOwnerStacks)
? ''
: ' in div (at **)\n') +
' in Parent (at **)',
: '\n in div (at **)' +
'\n in WrapperComponent (at **)' +
'\n in div (at **)' +
'\n in Parent (at **)'),
);
});

Expand Down Expand Up @@ -292,12 +293,13 @@ describe('ReactMultiChild', () => {
'across updates. Non-unique keys may cause children to be ' +
'duplicated and/or omitted — the behavior is unsupported and ' +
'could change in a future version.\n' +
' in div (at **)\n' +
' in WrapperComponent (at **)\n' +
' in div (at **)' +
(gate(flags => flags.enableOwnerStacks)
? ''
: ' in div (at **)\n') +
' in Parent (at **)',
: '\n in div (at **)' +
'\n in WrapperComponent (at **)' +
'\n in div (at **)' +
'\n in Parent (at **)'),
);
});
});
Expand Down
141 changes: 86 additions & 55 deletions packages/react-reconciler/src/ReactChildFiber.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,11 @@ let didWarnAboutGenerators;
let ownerHasKeyUseWarning;
let ownerHasFunctionTypeWarning;
let ownerHasSymbolTypeWarning;
let warnForMissingKey = (child: mixed, returnFiber: Fiber) => {};
let warnForMissingKey = (
returnFiber: Fiber,
workInProgress: Fiber,
child: mixed,
) => {};

if (__DEV__) {
didWarnAboutMaps = false;
Expand All @@ -108,7 +112,11 @@ if (__DEV__) {
ownerHasFunctionTypeWarning = ({}: {[string]: boolean});
ownerHasSymbolTypeWarning = ({}: {[string]: boolean});

warnForMissingKey = (child: mixed, returnFiber: Fiber) => {
warnForMissingKey = (
returnFiber: Fiber,
workInProgress: Fiber,
child: mixed,
) => {
if (child === null || typeof child !== 'object') {
return;
}
Expand Down Expand Up @@ -172,14 +180,7 @@ if (__DEV__) {
}
}

// We create a fake Fiber for the child to log the stack trace from.
// TODO: Refactor the warnForMissingKey calls to happen after fiber creation
// so that we can get access to the fiber that will eventually be created.
// That way the log can show up associated with the right instance in DevTools.
const fiber = createFiberFromElement((child: any), returnFiber.mode, 0);
fiber.return = returnFiber;

runWithFiberInDEV(fiber, () => {
runWithFiberInDEV(workInProgress, () => {
console.error(
'Each child in a list should have a unique "key" prop.' +
'%s%s See https://react.dev/link/warning-keys for more information.',
Expand Down Expand Up @@ -1034,9 +1035,10 @@ function createChildReconciler(
* Warns if there is a duplicate or missing key
*/
function warnOnInvalidKey(
returnFiber: Fiber,
workInProgress: Fiber,
child: mixed,
knownKeys: Set<string> | null,
returnFiber: Fiber,
): Set<string> | null {
if (__DEV__) {
if (typeof child !== 'object' || child === null) {
Expand All @@ -1045,7 +1047,7 @@ function createChildReconciler(
switch (child.$$typeof) {
case REACT_ELEMENT_TYPE:
case REACT_PORTAL_TYPE:
warnForMissingKey(child, returnFiber);
warnForMissingKey(returnFiber, workInProgress, child);
const key = child.key;
if (typeof key !== 'string') {
break;
Expand All @@ -1059,14 +1061,16 @@ function createChildReconciler(
knownKeys.add(key);
break;
}
console.error(
'Encountered two children with the same key, `%s`. ' +
'Keys should be unique so that components maintain their identity ' +
'across updates. Non-unique keys may cause children to be ' +
'duplicated and/or omitted — the behavior is unsupported and ' +
'could change in a future version.',
key,
);
runWithFiberInDEV(workInProgress, () => {
console.error(
'Encountered two children with the same key, `%s`. ' +
'Keys should be unique so that components maintain their identity ' +
'across updates. Non-unique keys may cause children to be ' +
'duplicated and/or omitted — the behavior is unsupported and ' +
'could change in a future version.',
key,
);
});
break;
case REACT_LAZY_TYPE: {
let resolvedChild;
Expand All @@ -1077,7 +1081,12 @@ function createChildReconciler(
const init = (child._init: any);
resolvedChild = init(payload);
}
warnOnInvalidKey(resolvedChild, knownKeys, returnFiber);
warnOnInvalidKey(
returnFiber,
workInProgress,
resolvedChild,
knownKeys,
);
break;
}
default:
Expand Down Expand Up @@ -1113,14 +1122,7 @@ function createChildReconciler(
// If you change this code, also update reconcileChildrenIterator() which
// uses the same algorithm.

if (__DEV__) {
// First, validate keys.
let knownKeys: Set<string> | null = null;
for (let i = 0; i < newChildren.length; i++) {
const child = newChildren[i];
knownKeys = warnOnInvalidKey(child, knownKeys, returnFiber);
}
}
let knownKeys: Set<string> | null = null;

let resultingFirstChild: Fiber | null = null;
let previousNewFiber: Fiber | null = null;
Expand Down Expand Up @@ -1153,6 +1155,16 @@ function createChildReconciler(
}
break;
}

if (__DEV__) {
knownKeys = warnOnInvalidKey(
returnFiber,
newFiber,
newChildren[newIdx],
knownKeys,
);
}

if (shouldTrackSideEffects) {
if (oldFiber && newFiber.alternate === null) {
// We matched the slot, but we didn't reuse the existing fiber, so we
Expand Down Expand Up @@ -1198,6 +1210,14 @@ function createChildReconciler(
if (newFiber === null) {
continue;
}
if (__DEV__) {
knownKeys = warnOnInvalidKey(
returnFiber,
newFiber,
newChildren[newIdx],
knownKeys,
);
}
lastPlacedIndex = placeChild(newFiber, lastPlacedIndex, newIdx);
if (previousNewFiber === null) {
// TODO: Move out of the loop. This only happens for the first run.
Expand Down Expand Up @@ -1228,6 +1248,14 @@ function createChildReconciler(
debugInfo,
);
if (newFiber !== null) {
if (__DEV__) {
knownKeys = warnOnInvalidKey(
returnFiber,
newFiber,
newChildren[newIdx],
knownKeys,
);
}
if (shouldTrackSideEffects) {
if (newFiber.alternate !== null) {
// The new fiber is a work in progress, but if there exists a
Expand Down Expand Up @@ -1410,17 +1438,10 @@ function createChildReconciler(
let knownKeys: Set<string> | null = null;

let step = newChildren.next();
if (__DEV__) {
knownKeys = warnOnInvalidKey(step.value, knownKeys, returnFiber);
}
for (
;
oldFiber !== null && !step.done;
newIdx++,
step = newChildren.next(),
knownKeys = __DEV__
? warnOnInvalidKey(step.value, knownKeys, returnFiber)
: null
newIdx++, step = newChildren.next()
) {
if (oldFiber.index > newIdx) {
nextOldFiber = oldFiber;
Expand All @@ -1445,6 +1466,16 @@ function createChildReconciler(
}
break;
}

if (__DEV__) {
knownKeys = warnOnInvalidKey(
returnFiber,
newFiber,
step.value,
knownKeys,
);
}

if (shouldTrackSideEffects) {
if (oldFiber && newFiber.alternate === null) {
// We matched the slot, but we didn't reuse the existing fiber, so we
Expand Down Expand Up @@ -1480,19 +1511,19 @@ function createChildReconciler(
if (oldFiber === null) {
// 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(),
knownKeys = __DEV__
? warnOnInvalidKey(step.value, knownKeys, returnFiber)
: null
) {
for (; !step.done; newIdx++, step = newChildren.next()) {
const newFiber = createChild(returnFiber, step.value, lanes, debugInfo);
if (newFiber === null) {
continue;
}
if (__DEV__) {
knownKeys = warnOnInvalidKey(
returnFiber,
newFiber,
step.value,
knownKeys,
);
}
lastPlacedIndex = placeChild(newFiber, lastPlacedIndex, newIdx);
if (previousNewFiber === null) {
// TODO: Move out of the loop. This only happens for the first run.
Expand All @@ -1513,15 +1544,7 @@ function createChildReconciler(
const existingChildren = mapRemainingChildren(oldFiber);

// Keep scanning and use the map to restore deleted items as moves.
for (
;
!step.done;
newIdx++,
step = newChildren.next(),
knownKeys = __DEV__
? warnOnInvalidKey(step.value, knownKeys, returnFiber)
: null
) {
for (; !step.done; newIdx++, step = newChildren.next()) {
const newFiber = updateFromMap(
existingChildren,
returnFiber,
Expand All @@ -1531,6 +1554,14 @@ function createChildReconciler(
debugInfo,
);
if (newFiber !== null) {
if (__DEV__) {
knownKeys = warnOnInvalidKey(
returnFiber,
newFiber,
step.value,
knownKeys,
);
}
if (shouldTrackSideEffects) {
if (newFiber.alternate !== null) {
// The new fiber is a work in progress, but if there exists a
Expand Down
4 changes: 1 addition & 3 deletions packages/react/src/__tests__/ReactJSXElementValidator-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -322,9 +322,7 @@ describe('ReactJSXElementValidator', () => {
</>,
);
});
}).toErrorDev('Encountered two children with the same key, `a`.', {
withoutStack: true,
});
}).toErrorDev('Encountered two children with the same key, `a`.');
});

it('does not call lazy initializers eagerly', () => {
Expand Down

0 comments on commit a0a435d

Please sign in to comment.