Skip to content

Commit

Permalink
Switch to binding the console with badging instead of calling it dire…
Browse files Browse the repository at this point in the history
…ctly (#30461)

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.

<img width="421" alt="Screenshot 2024-07-25 at 11 49 39 AM"
src="https://github.com/user-attachments/assets/81334c2f-e19e-476a-871e-c4db9dee294e">

When you expand to show ignore listed frames a ton of other frames show
up.

<img width="516" alt="Screenshot 2024-07-25 at 11 49 47 AM"
src="https://github.com/user-attachments/assets/2ab8bdfb-464c-408d-9176-ee2fabc114b6">

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.

<img width="590" alt="Screenshot 2024-07-25 at 12 09 09 PM"
src="https://github.com/user-attachments/assets/b8365d53-31f3-43df-abce-172d608d3c9c">

Am I wiling to eat the added complexity and slightly slower performance
for this nit? Definitely.
  • Loading branch information
sebmarkbage authored Jul 25, 2024
1 parent 6696853 commit e8df0cf
Show file tree
Hide file tree
Showing 11 changed files with 56 additions and 38 deletions.
18 changes: 11 additions & 7 deletions packages/react-client/src/ReactClientConsoleConfigBrowser.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,22 @@ const badgeStyle =
const resetStyle = '';
const pad = ' ';

export function printToConsole(
const bind = Function.prototype.bind;

export function bindToConsole(
methodName: string,
args: Array<any>,
badgeName: string,
): void {
): () => any {
let offset = 0;
switch (methodName) {
case 'dir':
case 'dirxml':
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.
Expand Down Expand Up @@ -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
}
18 changes: 11 additions & 7 deletions packages/react-client/src/ReactClientConsoleConfigPlain.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,21 +10,22 @@
const badgeFormat = '[%s] ';
const pad = ' ';

export function printToConsole(
const bind = Function.prototype.bind;

export function bindToConsole(
methodName: string,
args: Array<any>,
badgeName: string,
): void {
): () => any {
let offset = 0;
switch (methodName) {
case 'dir':
case 'dirxml':
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.
Expand All @@ -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
}
18 changes: 11 additions & 7 deletions packages/react-client/src/ReactClientConsoleConfigServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,21 +21,22 @@ const badgeStyle =
const resetStyle = '';
const pad = ' ';

export function printToConsole(
const bind = Function.prototype.bind;

export function bindToConsole(
methodName: string,
args: Array<any>,
badgeName: string,
): void {
): () => any {
let offset = 0;
switch (methodName) {
case 'dir':
case 'dirxml':
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.
Expand Down Expand Up @@ -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
}
6 changes: 3 additions & 3 deletions packages/react-client/src/ReactFlightClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,7 @@ import {
readFinalStringChunk,
createStringDecoder,
prepareDestinationForModule,
printToConsole,
bindToConsole,
} from './ReactFlightClientConfig';

import {registerServerReference} from './ReactFlightReplyClient';
Expand Down Expand Up @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
9 changes: 6 additions & 3 deletions packages/react-noop-renderer/src/ReactNoopFlightClient.js
Original file line number Diff line number Diff line change
Expand Up @@ -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),
);
},
});

Expand Down
9 changes: 6 additions & 3 deletions packages/react-noop-renderer/src/createReactNoop.js
Original file line number Diff line number Diff line change
Expand Up @@ -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),
);
},
};

Expand Down
6 changes: 3 additions & 3 deletions packages/react-reconciler/src/ReactFiberErrorLogger.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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',
Expand All @@ -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',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions packages/react-server/src/ReactFizzServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ import {
resetResumableState,
completeResumableState,
emitEarlyPreloads,
printToConsole,
bindToConsole,
} from './ReactFizzConfig';
import {
constructClassInstance,
Expand Down Expand Up @@ -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
}
Expand Down
2 changes: 1 addition & 1 deletion packages/react-server/src/forks/ReactFizzConfig.custom.js
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ export const supportsClientAPIs = true;
export const supportsRequestStorage = false;
export const requestStorage: AsyncLocalStorage<Request | void> = (null: any);

export const printToConsole = $$$config.printToConsole;
export const bindToConsole = $$$config.bindToConsole;

export const resetResumableState = $$$config.resetResumableState;
export const completeResumableState = $$$config.completeResumableState;
Expand Down

0 comments on commit e8df0cf

Please sign in to comment.