From 19bd26beb689e554fceb0b929dc5199be8cba594 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 15 Aug 2024 11:04:53 -0400 Subject: [PATCH] [Flight/DevTools] Pass the Server Component's "key" as Part of the ReactComponentInfo (#30703) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Supports showing the key in DevTools on the Server Component that the key was applied to. We can also use this to reconcile to preserve instance equality when they're reordered. One thing that's a bit weird about this is that if you provide an explicit key on a Server Component that alone doesn't have any semantics. It's because we pass the key down and let the nearest child inherit the key or get prefixed by the key. So you might see the same key as a prefix on the child of the Server Component too which might be a bit confusing. We could remove the prefix from children but that might also be a bit confusing if they collide. The div in this case doesn't have a key explicitly specified. It gets it from the Server Component parent. Screenshot 2024-08-14 at 10 06 36 PM Overall keys get a bit confusing when you apply filter. Especially since it's so common to actually apply the key on a Host Instance. So you often don't see the key. --- .../src/__tests__/ReactFlight-test.js | 12 +++++ .../src/__tests__/store-test.js | 18 +++---- .../src/backend/fiber/renderer.js | 51 +++++++++++++------ .../src/__tests__/ReactFlightDOMEdge-test.js | 4 +- .../react-server/src/ReactFlightServer.js | 5 ++ packages/shared/ReactTypes.js | 1 + 6 files changed, 64 insertions(+), 27 deletions(-) diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index 3d72f9c178aa9..c3ab6a46805f4 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -299,6 +299,7 @@ describe('ReactFlight', () => { { name: 'Greeting', env: 'Server', + key: null, owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' @@ -337,6 +338,7 @@ describe('ReactFlight', () => { { name: 'Greeting', env: 'Server', + key: null, owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' @@ -2614,6 +2616,7 @@ describe('ReactFlight', () => { { name: 'ServerComponent', env: 'Server', + key: null, owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' @@ -2631,6 +2634,7 @@ describe('ReactFlight', () => { { name: 'ThirdPartyComponent', env: 'third-party', + key: null, owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' @@ -2645,6 +2649,7 @@ describe('ReactFlight', () => { { name: 'ThirdPartyLazyComponent', env: 'third-party', + key: null, owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in myLazy (at **)\n in lazyInitializer (at **)' @@ -2659,6 +2664,7 @@ describe('ReactFlight', () => { { name: 'ThirdPartyFragmentComponent', env: 'third-party', + key: '3', owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' @@ -2732,6 +2738,7 @@ describe('ReactFlight', () => { { name: 'ServerComponent', env: 'Server', + key: null, owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' @@ -2748,6 +2755,7 @@ describe('ReactFlight', () => { { name: 'Keyed', env: 'Server', + key: 'keyed', owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in ServerComponent (at **)' @@ -2763,6 +2771,7 @@ describe('ReactFlight', () => { { name: 'ThirdPartyAsyncIterableComponent', env: 'third-party', + key: null, owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' @@ -2920,6 +2929,7 @@ describe('ReactFlight', () => { { name: 'Component', env: 'A', + key: null, owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' @@ -3040,6 +3050,7 @@ describe('ReactFlight', () => { const greetInfo = { name: 'Greeting', env: 'Server', + key: null, owner: null, stack: gate(flag => flag.enableOwnerStacks) ? ' in Object. (at **)' @@ -3050,6 +3061,7 @@ describe('ReactFlight', () => { { name: 'Container', env: 'Server', + key: null, owner: greetInfo, stack: gate(flag => flag.enableOwnerStacks) ? ' in Greeting (at **)' diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index 2d3b6ed0cc0ee..cebc7e0bbcb82 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -2439,7 +2439,7 @@ describe('Store', () => { }); // @reactVersion > 18.2 - it('can reorder keyed components', async () => { + it('can reorder keyed server components', async () => { function ClientComponent({text}) { return
{text}
; } @@ -2452,9 +2452,7 @@ describe('Store', () => { name: 'ServerComponent', env: 'Server', owner: null, - // TODO: Ideally the debug info should include the "key" too to - // preserve the virtual identity of the server component when - // reordered. Atm only the children of it gets reparented. + key: key, }, ]; return ServerPromise; @@ -2468,11 +2466,11 @@ describe('Store', () => { expect(store).toMatchInlineSnapshot(` [root] ▾ - ▾ [Server] + ▾ [Server] - ▾ [Server] + ▾ [Server] - ▾ [Server] + ▾ [Server] `); @@ -2480,11 +2478,11 @@ describe('Store', () => { expect(store).toMatchInlineSnapshot(` [root] ▾ - ▾ [Server] + ▾ [Server] - ▾ [Server] + ▾ [Server] - ▾ [Server] + ▾ [Server] `); }); diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 18aaf33a065ca..a48b22f647548 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -2220,9 +2220,12 @@ export function attach( const isProfilingSupported = false; // TODO: Support Tree Base Duration Based on Children. - const key = null; // TODO: Track keys on ReactComponentInfo; - const env = instance.data.env; - let displayName = instance.data.name || ''; + const componentInfo = instance.data; + + const key = + typeof componentInfo.key === 'string' ? componentInfo.key : null; + const env = componentInfo.env; + let displayName = componentInfo.name || ''; if (typeof env === 'string') { // We model environment as an HoC name for now. displayName = env + '(' + displayName + ')'; @@ -2855,19 +2858,35 @@ export function attach( ); } } - const firstRemainingChild = remainingReconcilingChildren; + // TODO: Find the best matching existing child based on the key if defined. + + let bestMatch = remainingReconcilingChildren; + if (componentInfo.key != null) { + // If there is a key try to find a matching key in the set. + bestMatch = remainingReconcilingChildren; + while (bestMatch !== null) { + if ( + bestMatch.kind === VIRTUAL_INSTANCE && + bestMatch.data.key === componentInfo.key + ) { + break; + } + bestMatch = bestMatch.nextSibling; + } + } if ( - firstRemainingChild !== null && - firstRemainingChild.kind === VIRTUAL_INSTANCE && - firstRemainingChild.data.name === componentInfo.name && - firstRemainingChild.data.env === componentInfo.env + bestMatch !== null && + bestMatch.kind === VIRTUAL_INSTANCE && + bestMatch.data.name === componentInfo.name && + bestMatch.data.env === componentInfo.env && + bestMatch.data.key === componentInfo.key ) { // If the previous children had a virtual instance in the same slot // with the same name, then we claim it and reuse it for this update. // Update it with the latest entry. - firstRemainingChild.data = componentInfo; - moveChild(firstRemainingChild); - previousVirtualInstance = firstRemainingChild; + bestMatch.data = componentInfo; + moveChild(bestMatch); + previousVirtualInstance = bestMatch; previousVirtualInstanceWasMount = false; } else { // Otherwise we create a new instance. @@ -4321,11 +4340,13 @@ export function attach( ): InspectedElement | null { const canViewSource = false; - const key = null; // TODO: Track keys on ReactComponentInfo; + const componentInfo = virtualInstance.data; + const key = + typeof componentInfo.key === 'string' ? componentInfo.key : null; const props = null; // TODO: Track props on ReactComponentInfo; - const env = virtualInstance.data.env; - let displayName = virtualInstance.data.name || ''; + const env = componentInfo.env; + let displayName = componentInfo.name || ''; if (typeof env === 'string') { // We model environment as an HoC name for now. displayName = env + '(' + displayName + ')'; @@ -4384,7 +4405,7 @@ export function attach( // Does the component have legacy context attached to it. hasLegacyContext: false, - key: key != null ? key : null, + key: key, displayName: displayName, type: ElementTypeVirtual, diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js index 9d4a123ac3b75..ffef621e9761b 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMEdge-test.js @@ -307,7 +307,7 @@ describe('ReactFlightDOMEdge', () => { const serializedContent = await readResult(stream1); - expect(serializedContent.length).toBeLessThan(410); + expect(serializedContent.length).toBeLessThan(425); expect(timesRendered).toBeLessThan(5); const model = await ReactServerDOMClient.createFromReadableStream(stream2, { @@ -374,7 +374,7 @@ describe('ReactFlightDOMEdge', () => { const [stream1, stream2] = passThrough(stream).tee(); const serializedContent = await readResult(stream1); - expect(serializedContent.length).toBeLessThan(__DEV__ ? 590 : 400); + expect(serializedContent.length).toBeLessThan(__DEV__ ? 605 : 400); expect(timesRendered).toBeLessThan(5); const model = await ReactServerDOMClient.createFromReadableStream(stream2, { diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 6c9536d95acf7..0f3c53960e4df 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -970,6 +970,7 @@ function callWithDebugContextInDEV( const componentDebugInfo: ReactComponentInfo = { name: '', env: task.environmentName, + key: null, owner: task.debugOwner, }; if (enableOwnerStacks) { @@ -1036,6 +1037,7 @@ function renderFunctionComponent( componentDebugInfo = ({ name: componentName, env: componentEnv, + key: key, owner: task.debugOwner, }: ReactComponentInfo); if (enableOwnerStacks) { @@ -1575,6 +1577,7 @@ function renderElement( const componentDebugInfo: ReactComponentInfo = { name: 'Fragment', env: (0, request.environmentName)(), + key: key, owner: task.debugOwner, stack: task.debugStack === null @@ -2615,6 +2618,7 @@ function renderModelDestructive( > = { name: (value: any).name, env: (value: any).env, + key: (value: any).key, owner: (value: any).owner, }; if (enableOwnerStacks) { @@ -3287,6 +3291,7 @@ function renderConsoleValue( > = { name: (value: any).name, env: (value: any).env, + key: (value: any).key, owner: (value: any).owner, }; if (enableOwnerStacks) { diff --git a/packages/shared/ReactTypes.js b/packages/shared/ReactTypes.js index dd7c5c22743a1..7d51fbe4725d3 100644 --- a/packages/shared/ReactTypes.js +++ b/packages/shared/ReactTypes.js @@ -190,6 +190,7 @@ export type ReactStackTrace = Array; export type ReactComponentInfo = { +name?: string, +env?: string, + +key?: null | string, +owner?: null | ReactComponentInfo, +stack?: null | ReactStackTrace, // Stashed Data for the Specific Execution Environment. Not part of the transport protocol