Skip to content

Commit

Permalink
Omit the component stack from the replaying
Browse files Browse the repository at this point in the history
Since getCurrentStack is now implemented, React appends component stacks
to its own logs. However, at the same time we've instrumented the console
so we need to strip them back out before sending to the client.

This is another reason we shouldn't append them from React.
  • Loading branch information
sebmarkbage committed Jul 3, 2024
1 parent aeffcc2 commit b0becf2
Show file tree
Hide file tree
Showing 3 changed files with 66 additions and 2 deletions.
42 changes: 42 additions & 0 deletions packages/react-client/src/__tests__/ReactFlight-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ let ErrorBoundary;
let NoErrorExpected;
let Scheduler;
let assertLog;
let assertConsoleErrorDev;

describe('ReactFlight', () => {
beforeEach(() => {
Expand All @@ -102,6 +103,7 @@ describe('ReactFlight', () => {
Scheduler = require('scheduler');
const InternalTestUtils = require('internal-test-utils');
assertLog = InternalTestUtils.assertLog;
assertConsoleErrorDev = InternalTestUtils.assertConsoleErrorDev;

ErrorBoundary = class extends React.Component {
state = {hasError: false, error: null};
Expand Down Expand Up @@ -2758,4 +2760,44 @@ describe('ReactFlight', () => {
'\n in Bar (at **)' + '\n in Foo (at **)',
);
});

// @gate (enableOwnerStacks && enableServerComponentLogs) || !__DEV__
it('should not include component stacks in replayed logs (unless DevTools add them)', () => {
function Foo() {
return 'hi';
}

function Bar() {
const array = [];
// Trigger key warning
array.push(ReactServer.createElement(Foo));
return ReactServer.createElement('div', null, array);
}

function App() {
return ReactServer.createElement(Bar);
}

const transport = ReactNoopFlightServer.render(
ReactServer.createElement(App),
);
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 **)',
]);

// Replay logs on the client
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.',
],
// We should not have a stack in the replay because that should be added either by console.createTask
// or React DevTools on the client. Neither of which we do here.
{withoutStack: true},
);
});
});
22 changes: 20 additions & 2 deletions packages/react-server/src/ReactFlightServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ import {resolveOwner, setCurrentOwner} from './flight/ReactFlightCurrentOwner';

import {getOwnerStackByComponentInfoInDev} from './flight/ReactFlightComponentStack';

import {isWritingAppendedStack} from 'shared/consoleWithStackDev';

import {
getIteratorFn,
REACT_ELEMENT_TYPE,
Expand Down Expand Up @@ -265,8 +267,9 @@ function patchConsole(consoleInst: typeof console, methodName: string) {
'name',
);
const wrapperMethod = function (this: typeof console) {
let args = arguments;
const request = resolveRequest();
if (methodName === 'assert' && arguments[0]) {
if (methodName === 'assert' && args[0]) {
// assert doesn't emit anything unless first argument is falsy so we can skip it.
} else if (request !== null) {
// Extract the stack. Not all console logs print the full stack but they have at
Expand All @@ -278,7 +281,22 @@ function patchConsole(consoleInst: typeof console, methodName: string) {
// refer to previous logs in debug info to associate them with a component.
const id = request.nextChunkId++;
const owner: null | ReactComponentInfo = resolveOwner();
emitConsoleChunk(request, id, methodName, owner, stack, arguments);
if (
isWritingAppendedStack &&
(methodName === 'error' || methodName === 'warn') &&
args.length > 1 &&
typeof args[0] === 'string' &&
args[0].endsWith('%s')
) {
// This looks like we've appended the component stack to the error from our own logs.
// We don't want those added to the replayed logs since those have the opportunity to add
// their own stacks or use console.createTask on the client as needed.
// TODO: Remove this special case once we remove consoleWithStackDev.
// $FlowFixMe[method-unbinding]
args = Array.prototype.slice.call(args, 0, args.length - 1);
args[0] = args[0].slice(0, args[0].length - 2);
}
emitConsoleChunk(request, id, methodName, owner, stack, args);
}
// $FlowFixMe[prop-missing]
return originalMethod.apply(this, arguments);
Expand Down
4 changes: 4 additions & 0 deletions packages/shared/consoleWithStackDev.js
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,8 @@ export function error(format, ...args) {
// eslint-disable-next-line react-internal/no-production-logging
const supportsCreateTask = __DEV__ && enableOwnerStacks && !!console.createTask;

export let isWritingAppendedStack = false;

function printWarning(level, format, args, currentStack) {
// When changing this logic, you might want to also
// update consoleWithStackDev.www.js as well.
Expand All @@ -50,6 +52,7 @@ function printWarning(level, format, args, currentStack) {
// can be lost while DevTools isn't open but we can't detect this.
const stack = ReactSharedInternals.getCurrentStack(currentStack);
if (stack !== '') {
isWritingAppendedStack = true;
format += '%s';
args = args.concat([stack]);
}
Expand All @@ -60,5 +63,6 @@ function printWarning(level, format, args, currentStack) {
// breaks IE9: https://github.com/facebook/react/issues/13610
// eslint-disable-next-line react-internal/no-production-logging
Function.prototype.apply.call(console[level], console, args);
isWritingAppendedStack = false;
}
}

0 comments on commit b0becf2

Please sign in to comment.