From a0a435d68f2e4e506faef4e763d13cbeb2e819c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 7 Jun 2024 13:38:44 -0400 Subject: [PATCH] [Fiber] Track the Real Fiber for Key Warnings (#29791) 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. --- .../src/__tests__/store-test.js | 11 +- .../__tests__/ReactChildReconciler-test.js | 2 + .../src/__tests__/ReactMultiChild-test.js | 18 ++- .../react-reconciler/src/ReactChildFiber.js | 141 +++++++++++------- .../ReactJSXElementValidator-test.js | 4 +- 5 files changed, 104 insertions(+), 72 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index c6ce366df04b2..6e065cdc93906 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -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 []; } @@ -1936,9 +1934,10 @@ describe('Store', () => { ); expect(store).toMatchInlineSnapshot(` + ✕ 1, ⚠ 0 [root] ▾ - + ✕ `); }); diff --git a/packages/react-dom/src/__tests__/ReactChildReconciler-test.js b/packages/react-dom/src/__tests__/ReactChildReconciler-test.js index 425f4abd13346..1c1475534f3e0 100644 --- a/packages/react-dom/src/__tests__/ReactChildReconciler-test.js +++ b/packages/react-dom/src/__tests__/ReactChildReconciler-test.js @@ -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) ? '' @@ -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) ? '' diff --git a/packages/react-dom/src/__tests__/ReactMultiChild-test.js b/packages/react-dom/src/__tests__/ReactMultiChild-test.js index 1ffc793f96023..087e92985057b 100644 --- a/packages/react-dom/src/__tests__/ReactMultiChild-test.js +++ b/packages/react-dom/src/__tests__/ReactMultiChild-test.js @@ -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 **)'), ); }); @@ -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 **)'), ); }); }); diff --git a/packages/react-reconciler/src/ReactChildFiber.js b/packages/react-reconciler/src/ReactChildFiber.js index 0220f988c15db..9334107301b0c 100644 --- a/packages/react-reconciler/src/ReactChildFiber.js +++ b/packages/react-reconciler/src/ReactChildFiber.js @@ -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; @@ -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; } @@ -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.', @@ -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 | null, - returnFiber: Fiber, ): Set | null { if (__DEV__) { if (typeof child !== 'object' || child === null) { @@ -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; @@ -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; @@ -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: @@ -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 | null = null; - for (let i = 0; i < newChildren.length; i++) { - const child = newChildren[i]; - knownKeys = warnOnInvalidKey(child, knownKeys, returnFiber); - } - } + let knownKeys: Set | null = null; let resultingFirstChild: Fiber | null = null; let previousNewFiber: Fiber | null = null; @@ -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 @@ -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. @@ -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 @@ -1410,17 +1438,10 @@ function createChildReconciler( let knownKeys: Set | 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; @@ -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 @@ -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. @@ -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, @@ -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 diff --git a/packages/react/src/__tests__/ReactJSXElementValidator-test.js b/packages/react/src/__tests__/ReactJSXElementValidator-test.js index 7ac4ead790f60..1eec2ef8656e3 100644 --- a/packages/react/src/__tests__/ReactJSXElementValidator-test.js +++ b/packages/react/src/__tests__/ReactJSXElementValidator-test.js @@ -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', () => {