From 65c50489c6790ba14d8b42da475f51c553aa15d4 Mon Sep 17 00:00:00 2001 From: Sebastian Markbage Date: Thu, 25 Jul 2024 18:42:43 -0400 Subject: [PATCH] Enable owner stacks on the client when replaying logs --- .../react-client/src/ReactFlightClient.js | 89 +++++++++++++------ .../src/__tests__/ReactFlight-test.js | 47 ++++++---- .../react-server/src/ReactFlightServer.js | 57 ++++++++---- .../ReactComponentInfoStack.js} | 0 4 files changed, 136 insertions(+), 57 deletions(-) rename packages/{react-server/src/flight/ReactFlightComponentStack.js => shared/ReactComponentInfoStack.js} (100%) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index ade358b4186a2..ad1c051408de1 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -73,8 +73,21 @@ import { import getComponentNameFromType from 'shared/getComponentNameFromType'; +import {getOwnerStackByComponentInfoInDev} from 'shared/ReactComponentInfoStack'; + import isArray from 'shared/isArray'; +import * as React from 'react'; + +// TODO: This is an unfortunate hack. We shouldn't feature detect the internals +// like this. It's just that for now we support the same build of the Flight +// client both in the RSC environment, in the SSR environments as well as the +// browser client. We should probably have a separate RSC build. This is DEV +// only though. +const ReactSharedInternals = + React.__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE || + React.__SERVER_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE; + export type {CallServerCallback, EncodeFormActionCallback}; interface FlightStreamController { @@ -2282,6 +2295,22 @@ function resolveDebugInfo( chunkDebugInfo.push(debugInfo); } +let currentOwnerInDEV: null | ReactComponentInfo = null; +function getCurrentStackInDEV(): string { + if (__DEV__) { + if (enableOwnerStacks) { + const owner: null | ReactComponentInfo = currentOwnerInDEV; + if (owner === null) { + return ''; + } + return getOwnerStackByComponentInfoInDev(owner); + } + // We don't have Parent Stacks in Flight. + return ''; + } + return ''; +} + function resolveConsoleEntry( response: Response, value: UninitializedModel, @@ -2310,34 +2339,44 @@ function resolveConsoleEntry( const owner = payload[2]; const env = payload[3]; const args = payload.slice(4); - if (!enableOwnerStacks) { - // Printing with stack isn't really limited to owner stacks but - // we gate it behind the same flag for now while iterating. - bindToConsole(methodName, args, env)(); - return; - } - const callStack = buildFakeCallStack( - response, - stackTrace, - env, - bindToConsole(methodName, args, env), - ); - if (owner != null) { - const task = initializeFakeTask(response, owner, env); - initializeFakeStack(response, owner); - if (task !== null) { - task.run(callStack); + + // There really shouldn't be anything else on the stack atm. + const prevStack = ReactSharedInternals.getCurrentStack; + ReactSharedInternals.getCurrentStack = getCurrentStackInDEV; + currentOwnerInDEV = owner; + + try { + if (!enableOwnerStacks) { + // Printing with stack isn't really limited to owner stacks but + // we gate it behind the same flag for now while iterating. + bindToConsole(methodName, args, env)(); return; } - // TODO: Set the current owner so that captureOwnerStack() adds the component - // stack during the replay - if needed. - } - const rootTask = getRootTask(response, env); - if (rootTask != null) { - rootTask.run(callStack); - return; + const callStack = buildFakeCallStack( + response, + stackTrace, + env, + bindToConsole(methodName, args, env), + ); + if (owner != null) { + const task = initializeFakeTask(response, owner, env); + initializeFakeStack(response, owner); + if (task !== null) { + task.run(callStack); + return; + } + // TODO: Set the current owner so that captureOwnerStack() adds the component + // stack during the replay - if needed. + } + const rootTask = getRootTask(response, env); + if (rootTask != null) { + rootTask.run(callStack); + return; + } + callStack(); + } finally { + ReactSharedInternals.getCurrentStack = prevStack; } - callStack(); } function mergeBuffer( diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index ce0d2861d668c..40d18b8e706c8 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -2918,7 +2918,7 @@ describe('ReactFlight', () => { expect(ReactNoop).toMatchRenderedOutput(
hi
); }); - // @gate enableServerComponentLogs && __DEV__ + // @gate enableServerComponentLogs && __DEV__ && enableOwnerStacks it('replays logs, but not onError logs', async () => { function foo() { return 'hello'; @@ -2928,12 +2928,21 @@ describe('ReactFlight', () => { throw new Error('err'); } + function App() { + return ReactServer.createElement(ServerComponent); + } + + let ownerStacks = []; + // These tests are specifically testing console.log. // Assign to `mockConsoleLog` so we can still inspect it when `console.log` // is overridden by the test modules. The original function will be restored // after this test finishes by `jest.restoreAllMocks()`. const mockConsoleLog = spyOnDevAndProd(console, 'log').mockImplementation( - () => {}, + () => { + // Uses server React. + ownerStacks.push(normalizeCodeLocInfo(ReactServer.captureOwnerStack())); + }, ); let transport; @@ -2946,14 +2955,20 @@ describe('ReactFlight', () => { ReactServer = require('react'); ReactNoopFlightServer = require('react-noop-renderer/flight-server'); transport = ReactNoopFlightServer.render({ - root: ReactServer.createElement(ServerComponent), + root: ReactServer.createElement(App), }); }).toErrorDev('err'); expect(mockConsoleLog).toHaveBeenCalledTimes(1); expect(mockConsoleLog.mock.calls[0][0]).toBe('hi'); expect(mockConsoleLog.mock.calls[0][1].prop).toBe(123); + expect(ownerStacks).toEqual(['\n in App (at **)']); mockConsoleLog.mockClear(); + mockConsoleLog.mockImplementation(() => { + // Switching to client React. + ownerStacks.push(normalizeCodeLocInfo(React.captureOwnerStack())); + }); + ownerStacks = []; // The error should not actually get logged because we're not awaiting the root // so it's not thrown but the server log also shouldn't be replayed. @@ -2973,6 +2988,8 @@ describe('ReactFlight', () => { expect(typeof loggedFn2).toBe('function'); expect(loggedFn2).not.toBe(foo); expect(loggedFn2.toString()).toBe(foo.toString()); + + expect(ownerStacks).toEqual(['\n in App (at **)']); }); it('uses the server component debug info as the element owner in DEV', async () => { @@ -3159,18 +3176,18 @@ describe('ReactFlight', () => { jest.resetModules(); jest.mock('react', () => React); ReactNoopFlightClient.read(transport); - assertConsoleErrorDev( - [ - 'Each child in a list should have a unique "key" prop.' + - ' See https://react.dev/link/warning-keys for more information.', - 'Error objects cannot be rendered as text children. Try formatting it using toString().\n' + - '
Womp womp: {Error}
\n' + - ' ^^^^^^^', - ], - // We should have a stack in the replay but we don't yet set the owner from the Flight replaying - // so our simulated polyfill doesn't end up getting any component stacks yet. - {withoutStack: true}, - ); + assertConsoleErrorDev([ + 'Each child in a list should have a unique "key" prop.' + + ' See https://react.dev/link/warning-keys for more information.\n' + + ' in Bar (at **)\n' + + ' in App (at **)', + 'Error objects cannot be rendered as text children. Try formatting it using toString().\n' + + '
Womp womp: {Error}
\n' + + ' ^^^^^^^\n' + + ' in Foo (at **)\n' + + ' in Bar (at **)\n' + + ' in App (at **)', + ]); }); it('can filter out stack frames of a serialized error in dev', async () => { diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index 3b39a4367b53d..dc17b8010883b 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -99,7 +99,7 @@ import {DefaultAsyncDispatcher} from './flight/ReactFlightAsyncDispatcher'; import {resolveOwner, setCurrentOwner} from './flight/ReactFlightCurrentOwner'; -import {getOwnerStackByComponentInfoInDev} from './flight/ReactFlightComponentStack'; +import {getOwnerStackByComponentInfoInDev} from 'shared/ReactComponentInfoStack'; import { callComponentInDEV, @@ -968,6 +968,7 @@ function callWithDebugContextInDEV( // a fake owner during this callback so we can get the stack trace from it. // This also gets sent to the client as the owner for the replaying log. const componentDebugInfo: ReactComponentInfo = { + name: '', env: task.environmentName, owner: task.debugOwner, }; @@ -2063,6 +2064,23 @@ function escapeStringValue(value: string): string { } } +function isReactComponentInfo(value: any): boolean { + // TODO: We don't currently have a brand check on ReactComponentInfo. Reconsider. + return ( + ((typeof value.debugTask === 'object' && + value.debugTask !== null && + // $FlowFixMe[method-unbinding] + typeof value.debugTask.run === 'function') || + value.debugStack instanceof Error) && + (enableOwnerStacks + ? isArray((value: any).stack) + : typeof (value: any).stack === 'undefined') && + typeof value.name === 'string' && + typeof value.env === 'string' && + value.owner !== undefined + ); +} + let modelRoot: null | ReactClientValue = false; function renderModel( @@ -2574,28 +2592,15 @@ function renderModelDestructive( ); } if (__DEV__) { - if ( - // TODO: We don't currently have a brand check on ReactComponentInfo. Reconsider. - ((typeof value.debugTask === 'object' && - value.debugTask !== null && - // $FlowFixMe[method-unbinding] - typeof value.debugTask.run === 'function') || - value.debugStack instanceof Error) && - (enableOwnerStacks - ? isArray((value: any).stack) - : typeof (value: any).stack === 'undefined') && - typeof value.name === 'string' && - typeof value.env === 'string' && - value.owner !== undefined - ) { + if (isReactComponentInfo(value)) { // This looks like a ReactComponentInfo. We can't serialize the ConsoleTask object so we // need to omit it before serializing. const componentDebugInfo: Omit< ReactComponentInfo, 'debugTask' | 'debugStack', > = { - name: value.name, - env: value.env, + name: (value: any).name, + env: (value: any).env, owner: (value: any).owner, }; if (enableOwnerStacks) { @@ -3259,6 +3264,24 @@ function renderConsoleValue( return Array.from((value: any)); } + if (isReactComponentInfo(value)) { + // This looks like a ReactComponentInfo. We can't serialize the ConsoleTask object so we + // need to omit it before serializing. + const componentDebugInfo: Omit< + ReactComponentInfo, + 'debugTask' | 'debugStack', + > = { + name: (value: any).name, + env: (value: any).env, + owner: (value: any).owner, + }; + if (enableOwnerStacks) { + // $FlowFixMe[cannot-write] + componentDebugInfo.stack = (value: any).stack; + } + return componentDebugInfo; + } + // $FlowFixMe[incompatible-return] return value; } diff --git a/packages/react-server/src/flight/ReactFlightComponentStack.js b/packages/shared/ReactComponentInfoStack.js similarity index 100% rename from packages/react-server/src/flight/ReactFlightComponentStack.js rename to packages/shared/ReactComponentInfoStack.js