[Flight/DevTools] Pass the Server Component's "key" as Part of the ReactComponentInfo (#30703)

sebmarkbage · web-flow · commit 19bd26beb689 · 2024-08-15T11:04:53.000-04:00
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. <img width="1107" alt="Screenshot 2024-08-14 at 10 06 36 PM" src="https://github.com/user-attachments/assets/cfc517cc-e737-44c3-a1be-050049267ee2"> 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.
diff --git 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.<anonymous> (at **)'
@@ -337,6 +338,7 @@ describe('ReactFlight', () => {
               {
                 name: 'Greeting',
                 env: 'Server',
+                key: null,
                 owner: null,
                 stack: gate(flag => flag.enableOwnerStacks)
                   ? '    in Object.<anonymous> (at **)'
@@ -2614,6 +2616,7 @@ describe('ReactFlight', () => {
               {
                 name: 'ServerComponent',
                 env: 'Server',
+                key: null,
                 owner: null,
                 stack: gate(flag => flag.enableOwnerStacks)
                   ? '    in Object.<anonymous> (at **)'
@@ -2631,6 +2634,7 @@ describe('ReactFlight', () => {
               {
                 name: 'ThirdPartyComponent',
                 env: 'third-party',
+                key: null,
                 owner: null,
                 stack: gate(flag => flag.enableOwnerStacks)
                   ? '    in Object.<anonymous> (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.<anonymous> (at **)'
@@ -2732,6 +2738,7 @@ describe('ReactFlight', () => {
               {
                 name: 'ServerComponent',
                 env: 'Server',
+                key: null,
                 owner: null,
                 stack: gate(flag => flag.enableOwnerStacks)
                   ? '    in Object.<anonymous> (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.<anonymous> (at **)'
@@ -2920,6 +2929,7 @@ describe('ReactFlight', () => {
               {
                 name: 'Component',
                 env: 'A',
+                key: null,
                 owner: null,
                 stack: gate(flag => flag.enableOwnerStacks)
                   ? '    in Object.<anonymous> (at **)'
@@ -3040,6 +3050,7 @@ describe('ReactFlight', () => {
         const greetInfo = {
           name: 'Greeting',
           env: 'Server',
+          key: null,
           owner: null,
           stack: gate(flag => flag.enableOwnerStacks)
             ? '    in Object.<anonymous> (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
@@ -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 <div>{text}</div>;
     }
@@ -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,23 +2466,23 @@ describe('Store', () => {
     expect(store).toMatchInlineSnapshot(`
       [root]
         ▾ <App>
-          ▾ <ServerComponent> [Server]
+          ▾ <ServerComponent key="A"> [Server]
               <ClientComponent key="A">
-          ▾ <ServerComponent> [Server]
+          ▾ <ServerComponent key="B"> [Server]
               <ClientComponent key="B">
-          ▾ <ServerComponent> [Server]
+          ▾ <ServerComponent key="C"> [Server]
               <ClientComponent key="C">
       `);
 
     await actAsync(() => render(<App initial={false} />));
     expect(store).toMatchInlineSnapshot(`
       [root]
         ▾ <App>
-          ▾ <ServerComponent> [Server]
+          ▾ <ServerComponent key="B"> [Server]
               <ClientComponent key="B">
-          ▾ <ServerComponent> [Server]
+          ▾ <ServerComponent key="A"> [Server]
               <ClientComponent key="A">
-          ▾ <ServerComponent> [Server]
+          ▾ <ServerComponent key="D"> [Server]
               <ClientComponent key="D">
       `);
   });
diff --git 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
@@ -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
@@ -970,6 +970,7 @@ function callWithDebugContextInDEV<A, T>(
   const componentDebugInfo: ReactComponentInfo = {
     name: '',
     env: task.environmentName,
+    key: null,
     owner: task.debugOwner,
   };
   if (enableOwnerStacks) {
@@ -1036,6 +1037,7 @@ function renderFunctionComponent<Props>(
       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
@@ -190,6 +190,7 @@ export type ReactStackTrace = Array<ReactCallSite>;
 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
