From e8df0cf9f7c7f641192f19841db9bf34b6a0abf7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Thu, 25 Jul 2024 12:32:16 -0400 Subject: [PATCH] Switch to binding the console with badging instead of calling it directly (#30461) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This is a major nit but this avoids an extra stack frame when we're replaying logs. Normally the `printToConsole` frame doesn't show up because it'd be ignore listed. Screenshot 2024-07-25 at 11 49 39 AM When you expand to show ignore listed frames a ton of other frames show up. Screenshot 2024-07-25 at 11 49 47 AM The annoying thing about this frame is that it's at the top of the stack where as typically framework stuff ends up at the bottom and something you can ignore. The user space stack comes first. With this fix there's no longer any `printToConsole` frame. Screenshot 2024-07-25 at 12 09 09 PM Am I wiling to eat the added complexity and slightly slower performance for this nit? Definitely. --- .../src/ReactClientConsoleConfigBrowser.js | 18 +++++++++++------- .../src/ReactClientConsoleConfigPlain.js | 18 +++++++++++------- .../src/ReactClientConsoleConfigServer.js | 18 +++++++++++------- packages/react-client/src/ReactFlightClient.js | 6 +++--- .../forks/ReactFlightClientConfig.custom.js | 2 +- .../src/ReactNoopFlightClient.js | 9 ++++++--- .../react-noop-renderer/src/createReactNoop.js | 9 ++++++--- .../src/ReactFiberErrorLogger.js | 6 +++--- .../src/forks/ReactFiberConfig.custom.js | 2 +- packages/react-server/src/ReactFizzServer.js | 4 ++-- .../src/forks/ReactFizzConfig.custom.js | 2 +- 11 files changed, 56 insertions(+), 38 deletions(-) diff --git a/packages/react-client/src/ReactClientConsoleConfigBrowser.js b/packages/react-client/src/ReactClientConsoleConfigBrowser.js index c1f525d28a8c7..355edc9c08197 100644 --- a/packages/react-client/src/ReactClientConsoleConfigBrowser.js +++ b/packages/react-client/src/ReactClientConsoleConfigBrowser.js @@ -20,11 +20,13 @@ const badgeStyle = const resetStyle = ''; const pad = ' '; -export function printToConsole( +const bind = Function.prototype.bind; + +export function bindToConsole( methodName: string, args: Array, badgeName: string, -): void { +): () => any { let offset = 0; switch (methodName) { case 'dir': @@ -32,9 +34,8 @@ export function printToConsole( case 'groupEnd': case 'table': { // These methods cannot be colorized because they don't take a formatting string. - // eslint-disable-next-line react-internal/no-production-logging - console[methodName].apply(console, args); - return; + // $FlowFixMe + return bind.apply(console[methodName], [console].concat(args)); // eslint-disable-line react-internal/no-production-logging } case 'assert': { // assert takes formatting options as the second argument. @@ -63,6 +64,9 @@ export function printToConsole( ); } - // $FlowFixMe[invalid-computed-prop] - console[methodName].apply(console, newArgs); // eslint-disable-line react-internal/no-production-logging + // The "this" binding in the "bind"; + newArgs.unshift(console); + + // $FlowFixMe + return bind.apply(console[methodName], newArgs); // eslint-disable-line react-internal/no-production-logging } diff --git a/packages/react-client/src/ReactClientConsoleConfigPlain.js b/packages/react-client/src/ReactClientConsoleConfigPlain.js index f2ec996b6af13..6b41ad4effe98 100644 --- a/packages/react-client/src/ReactClientConsoleConfigPlain.js +++ b/packages/react-client/src/ReactClientConsoleConfigPlain.js @@ -10,11 +10,13 @@ const badgeFormat = '[%s] '; const pad = ' '; -export function printToConsole( +const bind = Function.prototype.bind; + +export function bindToConsole( methodName: string, args: Array, badgeName: string, -): void { +): () => any { let offset = 0; switch (methodName) { case 'dir': @@ -22,9 +24,8 @@ export function printToConsole( case 'groupEnd': case 'table': { // These methods cannot be colorized because they don't take a formatting string. - // eslint-disable-next-line react-internal/no-production-logging - console[methodName].apply(console, args); - return; + // $FlowFixMe + return bind.apply(console[methodName], [console].concat(args)); // eslint-disable-line react-internal/no-production-logging } case 'assert': { // assert takes formatting options as the second argument. @@ -44,6 +45,9 @@ export function printToConsole( newArgs.splice(offset, 0, badgeFormat, pad + badgeName + pad); } - // $FlowFixMe[invalid-computed-prop] - console[methodName].apply(console, newArgs); // eslint-disable-line react-internal/no-production-logging + // The "this" binding in the "bind"; + newArgs.unshift(console); + + // $FlowFixMe + return bind.apply(console[methodName], newArgs); // eslint-disable-line react-internal/no-production-logging } diff --git a/packages/react-client/src/ReactClientConsoleConfigServer.js b/packages/react-client/src/ReactClientConsoleConfigServer.js index 19da98a176ada..efbcd2865d712 100644 --- a/packages/react-client/src/ReactClientConsoleConfigServer.js +++ b/packages/react-client/src/ReactClientConsoleConfigServer.js @@ -21,11 +21,13 @@ const badgeStyle = const resetStyle = ''; const pad = ' '; -export function printToConsole( +const bind = Function.prototype.bind; + +export function bindToConsole( methodName: string, args: Array, badgeName: string, -): void { +): () => any { let offset = 0; switch (methodName) { case 'dir': @@ -33,9 +35,8 @@ export function printToConsole( case 'groupEnd': case 'table': { // These methods cannot be colorized because they don't take a formatting string. - // eslint-disable-next-line react-internal/no-production-logging - console[methodName].apply(console, args); - return; + // $FlowFixMe + return bind.apply(console[methodName], [console].concat(args)); // eslint-disable-line react-internal/no-production-logging } case 'assert': { // assert takes formatting options as the second argument. @@ -64,6 +65,9 @@ export function printToConsole( ); } - // $FlowFixMe[invalid-computed-prop] - console[methodName].apply(console, newArgs); // eslint-disable-line react-internal/no-production-logging + // The "this" binding in the "bind"; + newArgs.unshift(console); + + // $FlowFixMe + return bind.apply(console[methodName], newArgs); // eslint-disable-line react-internal/no-production-logging } diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index dea171c51c04d..ade358b4186a2 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -56,7 +56,7 @@ import { readFinalStringChunk, createStringDecoder, prepareDestinationForModule, - printToConsole, + bindToConsole, } from './ReactFlightClientConfig'; import {registerServerReference} from './ReactFlightReplyClient'; @@ -2313,14 +2313,14 @@ function resolveConsoleEntry( if (!enableOwnerStacks) { // Printing with stack isn't really limited to owner stacks but // we gate it behind the same flag for now while iterating. - printToConsole(methodName, args, env); + bindToConsole(methodName, args, env)(); return; } const callStack = buildFakeCallStack( response, stackTrace, env, - printToConsole.bind(null, methodName, args, env), + bindToConsole(methodName, args, env), ); if (owner != null) { const task = initializeFakeTask(response, owner, env); diff --git a/packages/react-client/src/forks/ReactFlightClientConfig.custom.js b/packages/react-client/src/forks/ReactFlightClientConfig.custom.js index 88694f59324d4..d9b031c4b5fe1 100644 --- a/packages/react-client/src/forks/ReactFlightClientConfig.custom.js +++ b/packages/react-client/src/forks/ReactFlightClientConfig.custom.js @@ -48,4 +48,4 @@ export const createStringDecoder = $$$config.createStringDecoder; export const readPartialStringChunk = $$$config.readPartialStringChunk; export const readFinalStringChunk = $$$config.readFinalStringChunk; -export const printToConsole = $$$config.printToConsole; +export const bindToConsole = $$$config.bindToConsole; diff --git a/packages/react-noop-renderer/src/ReactNoopFlightClient.js b/packages/react-noop-renderer/src/ReactNoopFlightClient.js index a730270b46edc..456f8fd1bb461 100644 --- a/packages/react-noop-renderer/src/ReactNoopFlightClient.js +++ b/packages/react-noop-renderer/src/ReactNoopFlightClient.js @@ -45,9 +45,12 @@ const {createResponse, processBinaryChunk, getRoot, close} = ReactFlightClient({ parseModel(response: Response, json) { return JSON.parse(json, response._fromJSON); }, - printToConsole(methodName, args, badgeName) { - // eslint-disable-next-line react-internal/no-production-logging - console[methodName].apply(console, args); + bindToConsole(methodName, args, badgeName) { + return Function.prototype.bind.apply( + // eslint-disable-next-line react-internal/no-production-logging + console[methodName], + [console].concat(args), + ); }, }); diff --git a/packages/react-noop-renderer/src/createReactNoop.js b/packages/react-noop-renderer/src/createReactNoop.js index d88f7bd5a7f1b..4283e007fc00c 100644 --- a/packages/react-noop-renderer/src/createReactNoop.js +++ b/packages/react-noop-renderer/src/createReactNoop.js @@ -636,9 +636,12 @@ function createReactNoop(reconciler: Function, useMutation: boolean) { resetFormInstance(form: Instance) {}, - printToConsole(methodName, args, badgeName) { - // eslint-disable-next-line react-internal/no-production-logging - console[methodName].apply(console, args); + bindToConsole(methodName, args, badgeName) { + return Function.prototype.bind.apply( + // eslint-disable-next-line react-internal/no-production-logging + console[methodName], + [console].concat(args), + ); }, }; diff --git a/packages/react-reconciler/src/ReactFiberErrorLogger.js b/packages/react-reconciler/src/ReactFiberErrorLogger.js index b969b9c54cb19..2b53783a77daf 100644 --- a/packages/react-reconciler/src/ReactFiberErrorLogger.js +++ b/packages/react-reconciler/src/ReactFiberErrorLogger.js @@ -20,7 +20,7 @@ import ReactSharedInternals from 'shared/ReactSharedInternals'; import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; -import {printToConsole} from './ReactFiberConfig'; +import {bindToConsole} from './ReactFiberConfig'; // Side-channel since I'm not sure we want to make this part of the public API let componentName: null | string = null; @@ -117,7 +117,7 @@ export function defaultOnCaughtError( ) { // This was a Server error. We print the environment name in a badge just like we do with // replays of console logs to indicate that the source of this throw as actually the Server. - printToConsole( + bindToConsole( 'error', [ '%o\n\n%s\n\n%s\n', @@ -127,7 +127,7 @@ export function defaultOnCaughtError( // We let DevTools or console.createTask add the component stack to the end. ], error.environmentName, - ); + )(); } else { console.error( '%o\n\n%s\n\n%s\n', diff --git a/packages/react-reconciler/src/forks/ReactFiberConfig.custom.js b/packages/react-reconciler/src/forks/ReactFiberConfig.custom.js index 24c80469c72a5..59dae77ba1ed8 100644 --- a/packages/react-reconciler/src/forks/ReactFiberConfig.custom.js +++ b/packages/react-reconciler/src/forks/ReactFiberConfig.custom.js @@ -80,7 +80,7 @@ export const suspendInstance = $$$config.suspendInstance; export const waitForCommitToBeReady = $$$config.waitForCommitToBeReady; export const NotPendingTransition = $$$config.NotPendingTransition; export const resetFormInstance = $$$config.resetFormInstance; -export const printToConsole = $$$config.printToConsole; +export const bindToConsole = $$$config.bindToConsole; // ------------------- // Microtasks diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index f818f69da6d3d..a1ca5dbaa2838 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -80,7 +80,7 @@ import { resetResumableState, completeResumableState, emitEarlyPreloads, - printToConsole, + bindToConsole, } from './ReactFizzConfig'; import { constructClassInstance, @@ -386,7 +386,7 @@ function defaultErrorHandler(error: mixed) { ) { // This was a Server error. We print the environment name in a badge just like we do with // replays of console logs to indicate that the source of this throw as actually the Server. - printToConsole('error', [error], error.environmentName); + bindToConsole('error', [error], error.environmentName)(); } else { console['error'](error); // Don't transform to our wrapper } diff --git a/packages/react-server/src/forks/ReactFizzConfig.custom.js b/packages/react-server/src/forks/ReactFizzConfig.custom.js index 249292fd19519..02ce1e4e2afee 100644 --- a/packages/react-server/src/forks/ReactFizzConfig.custom.js +++ b/packages/react-server/src/forks/ReactFizzConfig.custom.js @@ -42,7 +42,7 @@ export const supportsClientAPIs = true; export const supportsRequestStorage = false; export const requestStorage: AsyncLocalStorage = (null: any); -export const printToConsole = $$$config.printToConsole; +export const bindToConsole = $$$config.bindToConsole; export const resetResumableState = $$$config.resetResumableState; export const completeResumableState = $$$config.completeResumableState;