Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Fiber] Track the Real Fiber for Key Warnings #29791

Merged
merged 3 commits into from
Jun 7, 2024
Merged

[Fiber] Track the Real Fiber for Key Warnings #29791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5740a0e
Refactor key warning to happen inline after we've matched a Fiber
sebmarkbage Jun 6, 2024
8eec615
We can also associate the duplicate key warning with this Fiber
sebmarkbage Jun 6, 2024
8b54f40
Update snapshot tests
sebmarkbage Jun 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions packages/react-devtools-shared/src/__tests__/store-test.js
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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