From 400e82227747f6b60fecbc3b43f7515b4fd89d8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 12 Jul 2024 13:02:22 -0400 Subject: [PATCH] Remove Component Stack from React Logged Warnings and Error Reporting (#30308) React transpiles some of its own `console.error` calls into a helper that appends component stacks to those calls. However, this doesn't cover user space `console.error` calls - which includes React helpers that React has moved into third parties like createClass and prop-types. The idea is that any user space component can add a warning just like React can which is why React DevTools adds them too if they don't already exist. Having them appended in both places is tricky because now you have to know whether to remove them from React's logs. Similarly it's often common for server-side frameworks to forget to cover the `console.error` logs from other sources since React DevTools isn't active there. However, it's also annoying to get component stacks clogging the terminal - depending on where the log came from. In the future `console.createTask()` will cover this use case natively and when available we don't append them at all. The new strategy relies on either: - React DevTools existing to add them to React logs as well as third parties. - `console.createTask` being supported and surfaced. - A third party framework showing the component stack either in an Error Dialog or appended to terminal output. For a third party to be able to implement this they need to be able to get the component stack. To get the component stack from within a `console.error` call you need to use the `React.captureOwnerStack()` helper which is only available in `enableOwnerStacks` flag. However, it's possible to polyfill with parent stacks using internals as a stop gap. There's a question of whether React 19 should just go out with `enableOwnerStacks` to expose this but regardless I think it's best it doesn't include component stacks from the runtime for consistency. In practice it's not really a regression though because typically either of the other options exists and error dialogs don't implement `console.error` overrides anyway yet. SSR terminals might miss them but they'd only have them in DEV warnings to begin with an a subset of React warnings. Typically those are either going to happen on the client anyway or replayed. Our tests are written to assert that component stacks work in various scenarios all over the place. To ensure that this keeps working I implement a "polyfill" that is similar to that expected a server framework might do - in `assertConsoleErrorDev` and `toErrorDev`. This PR doesn't yet change www or RN since they have their own forks of consoleWithStackDev for now. --- .../__tests__/ReactInternalTestUtils-test.js | 442 +++++++++++++++--- packages/internal-test-utils/consoleMock.js | 45 +- .../src/__tests__/ReactFlight-test.js | 26 +- .../ReactDOMConsoleErrorReporting-test.js | 18 +- ...eactDOMConsoleErrorReportingLegacy-test.js | 18 +- .../src/__tests__/ReactUpdates-test.js | 17 +- .../ReactIncrementalErrorLogging-test.js | 74 +-- .../react-server/src/ReactFlightServer.js | 22 +- .../createReactClassIntegration-test.js | 2 +- packages/shared/consoleWithStackDev.js | 31 +- .../shared/forks/consoleWithStackDev.rn.js | 4 - scripts/jest/matchers/toWarnDev.js | 28 ++ 12 files changed, 544 insertions(+), 183 deletions(-) diff --git a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js index ad3a7cdce7..befbdc617d 100644 --- a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js +++ b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js @@ -866,16 +866,40 @@ describe('ReactInternalTestUtils console assertions', () => { const message = expectToThrowFailure(() => { expect(root).toMatchRenderedOutput(
foobarbaz
); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.log was called without assertConsoleLogDev: - + Not asserted - + Not asserted - + Not asserted + console.log was called without assertConsoleLogDev: + + Not asserted + + Not asserted + + Not asserted - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + Not asserted + + Not asserted + + Not asserted + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + Not asserted + + Not asserted + + Not asserted + + You must call one of the assertConsoleDev helpers between each act call." + `); + } expect(root).toMatchRenderedOutput(
foobarbaz
); }); @@ -922,16 +946,52 @@ describe('ReactInternalTestUtils console assertions', () => { }); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.warn was called without assertConsoleWarnDev: - + A - + B - + C + console.warn was called without assertConsoleWarnDev: + + A + + B + + C - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } }); it('fails if act is called without any assertConsoleDev helpers', async () => { @@ -962,26 +1022,94 @@ describe('ReactInternalTestUtils console assertions', () => { }); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.log was called without assertConsoleLogDev: - + A - + B - + C + console.log was called without assertConsoleLogDev: + + A + + B + + C - console.warn was called without assertConsoleWarnDev: - + A - + B - + C + console.warn was called without assertConsoleWarnDev: + + A + + B + + C - console.error was called without assertConsoleErrorDev: - + A - + B - + C + console.error was called without assertConsoleErrorDev: + + A + + B + + C - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + A + + B + + C + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + A + + B + + C + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } }); // @gate __DEV__ @@ -1804,16 +1932,49 @@ describe('ReactInternalTestUtils console assertions', () => { const message = expectToThrowFailure(() => { expect(root).toMatchRenderedOutput(
foobarbaz
); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.warn was called without assertConsoleWarnDev: - + Not asserted - + Not asserted - + Not asserted + console.warn was called without assertConsoleWarnDev: + + Not asserted + + Not asserted + + Not asserted - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.warn was called without assertConsoleWarnDev: + + Not asserted%s, + + in Yield (at **) + + Not asserted%s, + + in Yield (at **) + + Not asserted%s, + + in Yield (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.warn was called without assertConsoleWarnDev: + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } expect(root).toMatchRenderedOutput(
foobarbaz
); }); @@ -1860,16 +2021,52 @@ describe('ReactInternalTestUtils console assertions', () => { }); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.error was called without assertConsoleErrorDev: - + A - + B - + C + console.error was called without assertConsoleErrorDev: + + A + + B + + C - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } }); it('fails if act is called without any assertConsoleDev helpers', async () => { @@ -1900,26 +2097,94 @@ describe('ReactInternalTestUtils console assertions', () => { }); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.log was called without assertConsoleLogDev: - + A - + B - + C + console.log was called without assertConsoleLogDev: + + A + + B + + C - console.warn was called without assertConsoleWarnDev: - + A - + B - + C + console.warn was called without assertConsoleWarnDev: + + A + + B + + C - console.error was called without assertConsoleErrorDev: - + A - + B - + C + console.error was called without assertConsoleErrorDev: + + A + + B + + C - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + A + + B + + C + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + A + + B + + C + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } }); // @gate __DEV__ @@ -2786,16 +3051,49 @@ describe('ReactInternalTestUtils console assertions', () => { const message = expectToThrowFailure(() => { expect(root).toMatchRenderedOutput(
foobarbaz
); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.error was called without assertConsoleErrorDev: - + Not asserted - + Not asserted - + Not asserted + console.error was called without assertConsoleErrorDev: + + Not asserted + + Not asserted + + Not asserted - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.error was called without assertConsoleErrorDev: + + Not asserted%s, + + in Yield (at **) + + Not asserted%s, + + in Yield (at **) + + Not asserted%s, + + in Yield (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.error was called without assertConsoleErrorDev: + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } expect(root).toMatchRenderedOutput(
foobarbaz
); }); diff --git a/packages/internal-test-utils/consoleMock.js b/packages/internal-test-utils/consoleMock.js index 3f35d9e122..6d03c74c1d 100644 --- a/packages/internal-test-utils/consoleMock.js +++ b/packages/internal-test-utils/consoleMock.js @@ -44,6 +44,34 @@ const patchConsoleMethod = ( return; } + // Append Component Stacks. Simulates a framework or DevTools appending them. + if ( + typeof format === 'string' && + (methodName === 'error' || methodName === 'warn') + ) { + const React = require('react'); + if (React.captureOwnerStack) { + // enableOwnerStacks enabled. When it's always on, we can assume this case. + const stack = React.captureOwnerStack(); + if (stack) { + format += '%s'; + args.push(stack); + } + } else { + // Otherwise we have to use internals to emulate parent stacks. + 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; + if (ReactSharedInternals && ReactSharedInternals.getCurrentStack) { + const stack = ReactSharedInternals.getCurrentStack(); + if (stack !== '') { + format += '%s'; + args.push(stack); + } + } + } + } + // Capture the call stack now so we can warn about it later. // The call stack has helpful information for the test author. // Don't throw yet though b'c it might be accidentally caught and suppressed. @@ -204,7 +232,7 @@ export function assertConsoleLogsCleared() { if (warnings.length > 0) { message += `\nconsole.warn was called without assertConsoleWarnDev:\n${diff( '', - warnings.join('\n'), + warnings.map(normalizeComponentStack).join('\n'), { omitAnnotationLines: true, }, @@ -213,7 +241,7 @@ export function assertConsoleLogsCleared() { if (errors.length > 0) { message += `\nconsole.error was called without assertConsoleErrorDev:\n${diff( '', - errors.join('\n'), + errors.map(normalizeComponentStack).join('\n'), { omitAnnotationLines: true, }, @@ -249,6 +277,19 @@ function normalizeCodeLocInfo(str) { }); } +function normalizeComponentStack(entry) { + if ( + typeof entry[0] === 'string' && + entry[0].endsWith('%s') && + isLikelyAComponentStack(entry[entry.length - 1]) + ) { + const clone = entry.slice(0); + clone[clone.length - 1] = normalizeCodeLocInfo(entry[entry.length - 1]); + return clone; + } + return entry; +} + const isLikelyAComponentStack = message => typeof message === 'string' && (message.indexOf('') > -1 || diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index af56b8c1a2..e48c926a91 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1436,12 +1436,23 @@ describe('ReactFlight', () => { it('should warn in DEV a child is missing keys on server component', () => { function NoKey({children}) { - return
; + return ReactServer.createElement('div', { + key: "this has a key but parent doesn't", + }); } expect(() => { + // While we're on the server we need to have the Server version active to track component stacks. + jest.resetModules(); + jest.mock('react', () => ReactServer); const transport = ReactNoopFlightServer.render( -
{Array(6).fill()}
, + ReactServer.createElement( + 'div', + null, + Array(6).fill(ReactServer.createElement(NoKey)), + ), ); + jest.resetModules(); + jest.mock('react', () => React); ReactNoopFlightClient.read(transport); }).toErrorDev('Each child in a list should have a unique "key" prop.'); }); @@ -2814,7 +2825,7 @@ describe('ReactFlight', () => { }); // @gate (enableOwnerStacks && enableServerComponentLogs) || !__DEV__ - it('should not include component stacks in replayed logs (unless DevTools add them)', () => { + it('should include only one component stack in replayed logs (if DevTools or polyfill adds them)', () => { class MyError extends Error { toJSON() { return 123; @@ -2839,6 +2850,9 @@ describe('ReactFlight', () => { return ReactServer.createElement(Bar); } + // While we're on the server we need to have the Server version active to track component stacks. + jest.resetModules(); + jest.mock('react', () => ReactServer); const transport = ReactNoopFlightServer.render( ReactServer.createElement(App), ); @@ -2857,6 +2871,8 @@ describe('ReactFlight', () => { ]); // Replay logs on the client + jest.resetModules(); + jest.mock('react', () => React); ReactNoopFlightClient.read(transport); assertConsoleErrorDev( [ @@ -2866,8 +2882,8 @@ describe('ReactFlight', () => { '
Womp womp: {Error}
\n' + ' ^^^^^^^', ], - // 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. + // 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}, ); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js index 9eeb4e7c07..2aca1e0588 100644 --- a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js @@ -143,7 +143,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining('%s'), expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -208,7 +209,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -274,7 +276,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining('%s'), expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -344,7 +347,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -410,7 +414,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining('%s'), expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -478,7 +483,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js index 65301c789d..99bfeac56b 100644 --- a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js @@ -162,7 +162,8 @@ describe('ReactDOMConsoleErrorReporting', () => { // Addendum by React: expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); @@ -239,7 +240,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -309,7 +311,8 @@ describe('ReactDOMConsoleErrorReporting', () => { // Addendum by React: expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); @@ -390,7 +393,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -460,7 +464,8 @@ describe('ReactDOMConsoleErrorReporting', () => { // Addendum by React: expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); @@ -540,7 +545,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index 1d71c71476..4916a72129 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -1857,12 +1857,14 @@ describe('ReactUpdates', () => { } let error = null; - let stack = null; + let ownerStack = null; let nativeStack = null; const originalConsoleError = console.error; - console.error = (e, s) => { + console.error = e => { error = e; - stack = s; + ownerStack = gate(flags => flags.enableOwnerStacks) + ? React.captureOwnerStack() + : null; nativeStack = new Error().stack; Scheduler.log('stop'); }; @@ -1878,12 +1880,11 @@ describe('ReactUpdates', () => { expect(error).toContain('Maximum update depth exceeded'); // The currently executing effect should be on the native stack expect(nativeStack).toContain('at myEffect'); - if (!gate(flags => flags.enableOwnerStacks)) { - // The currently running component's name is not in the owner - // stack because it's just its JSX callsite. - expect(stack).toContain('at NonTerminating'); + if (gate(flags => flags.enableOwnerStacks)) { + expect(ownerStack).toContain('at App'); + } else { + expect(ownerStack).toBe(null); } - expect(stack).toContain('at App'); }); it('can have nested updates if they do not cross the limit', async () => { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js index 8333445c69..6aa4034636 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js @@ -91,15 +91,16 @@ describe('ReactIncrementalErrorLogging', () => { 'Consider adding an error boundary to your tree ' + 'to customize error handling behavior.', ), - expect.stringMatching( - new RegExp( - gate(flags => flags.enableOwnerStacks) - ? '\\s+(in|at) ErrorThrowingComponent' - : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) div(.*)', - ), - ), + // The component stack is not added without the polyfill/devtools. + // expect.stringMatching( + // new RegExp( + // gate(flags => flags.enableOwnerStacks) + // ? '\\s+(in|at) ErrorThrowingComponent' + // : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + // '\\s+(in|at) span(.*)\n' + + // '\\s+(in|at) div(.*)', + // ), + // ), ); } }); @@ -139,15 +140,16 @@ describe('ReactIncrementalErrorLogging', () => { 'Consider adding an error boundary to your tree ' + 'to customize error handling behavior.', ), - expect.stringMatching( - new RegExp( - gate(flags => flags.enableOwnerStacks) - ? '\\s+(in|at) ErrorThrowingComponent' - : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) div(.*)', - ), - ), + // The component stack is not added without the polyfill/devtools. + // expect.stringMatching( + // new RegExp( + // gate(flags => flags.enableOwnerStacks) + // ? '\\s+(in|at) ErrorThrowingComponent' + // : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + // '\\s+(in|at) span(.*)\n' + + // '\\s+(in|at) div(.*)', + // ), + // ), ); } }); @@ -199,16 +201,17 @@ describe('ReactIncrementalErrorLogging', () => { 'React will try to recreate this component tree from scratch ' + 'using the error boundary you provided, ErrorBoundary.', ), - expect.stringMatching( - new RegExp( - gate(flags => flags.enableOwnerStacks) - ? '\\s+(in|at) ErrorThrowingComponent' - : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) ErrorBoundary(.*)\n' + - '\\s+(in|at) div(.*)', - ), - ), + // The component stack is not added without the polyfill/devtools. + // expect.stringMatching( + // new RegExp( + // gate(flags => flags.enableOwnerStacks) + // ? '\\s+(in|at) ErrorThrowingComponent' + // : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + // '\\s+(in|at) span(.*)\n' + + // '\\s+(in|at) ErrorBoundary(.*)\n' + + // '\\s+(in|at) div(.*)', + // ), + // ), ); } else { expect(logCapturedErrorCalls[0]).toEqual( @@ -282,13 +285,14 @@ describe('ReactIncrementalErrorLogging', () => { 'React will try to recreate this component tree from scratch ' + 'using the error boundary you provided, ErrorBoundary.', ), - expect.stringMatching( - gate(flag => flag.enableOwnerStacks) - ? new RegExp('\\s+(in|at) Foo') - : new RegExp( - '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)', - ), - ), + // The component stack is not added without the polyfill/devtools. + // expect.stringMatching( + // gate(flag => flag.enableOwnerStacks) + // ? new RegExp('\\s+(in|at) Foo') + // : new RegExp( + // '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)', + // ), + // ), ); } else { expect(console.error).toHaveBeenCalledWith( diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index a0d4b9e140..1eaf5369c7 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -99,8 +99,6 @@ import {resolveOwner, setCurrentOwner} from './flight/ReactFlightCurrentOwner'; import {getOwnerStackByComponentInfoInDev} from './flight/ReactFlightComponentStack'; -import {isWritingAppendedStack} from 'shared/consoleWithStackDev'; - import { getIteratorFn, REACT_ELEMENT_TYPE, @@ -267,9 +265,8 @@ function patchConsole(consoleInst: typeof console, methodName: string) { 'name', ); const wrapperMethod = function (this: typeof console) { - let args = arguments; const request = resolveRequest(); - if (methodName === 'assert' && args[0]) { + if (methodName === 'assert' && arguments[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 @@ -281,22 +278,7 @@ 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(); - 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); + emitConsoleChunk(request, id, methodName, owner, stack, arguments); } // $FlowFixMe[prop-missing] return originalMethod.apply(this, arguments); diff --git a/packages/react/src/__tests__/createReactClassIntegration-test.js b/packages/react/src/__tests__/createReactClassIntegration-test.js index 330150506c..811d897e85 100644 --- a/packages/react/src/__tests__/createReactClassIntegration-test.js +++ b/packages/react/src/__tests__/createReactClassIntegration-test.js @@ -804,7 +804,7 @@ describe('create-react-class-integration', () => { 'MyComponent: isMounted is deprecated. Instead, make sure to ' + 'clean up subscriptions and pending requests in componentWillUnmount ' + 'to prevent memory leaks.', - {withoutStack: true}, + // This now has a component stack even though it's part of a third-party library. ); // Dedupe diff --git a/packages/shared/consoleWithStackDev.js b/packages/shared/consoleWithStackDev.js index f709ca1e1f..b978b0b1d0 100644 --- a/packages/shared/consoleWithStackDev.js +++ b/packages/shared/consoleWithStackDev.js @@ -5,9 +5,6 @@ * LICENSE file in the root directory of this source tree. */ -import ReactSharedInternals from 'shared/ReactSharedInternals'; -import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; - export function setSuppressWarning(newSuppressWarning) { // TODO: Noop. Delete. } @@ -19,39 +16,25 @@ export function setSuppressWarning(newSuppressWarning) { // they are left as they are instead. export function warn(format, ...args) { - printWarning('warn', format, args, new Error('react-stack-top-frame')); + if (__DEV__) { + printWarning('warn', format, args); + } } export function error(format, ...args) { - printWarning('error', format, args, new Error('react-stack-top-frame')); + if (__DEV__) { + printWarning('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) { +function printWarning(level, format, args) { // When changing this logic, you might want to also // update consoleWithStackDev.www.js as well. if (__DEV__) { - if (!supportsCreateTask && ReactSharedInternals.getCurrentStack) { - // We only add the current stack to the console when createTask is not supported. - // Since createTask requires DevTools to be open to work, this means that stacks - // 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]); - } - } - args.unshift(format); // We intentionally don't use spread (or .apply) directly because it // 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; } } diff --git a/packages/shared/forks/consoleWithStackDev.rn.js b/packages/shared/forks/consoleWithStackDev.rn.js index 44767992cd..6ac30b39fb 100644 --- a/packages/shared/forks/consoleWithStackDev.rn.js +++ b/packages/shared/forks/consoleWithStackDev.rn.js @@ -36,14 +36,11 @@ export function error(format, ...args) { } } -export let isWritingAppendedStack = false; - function printWarning(level, format, args) { if (__DEV__) { if (ReactSharedInternals.getCurrentStack) { const stack = ReactSharedInternals.getCurrentStack(); if (stack !== '') { - isWritingAppendedStack = true; format += '%s'; args = args.concat([stack]); } @@ -54,6 +51,5 @@ function printWarning(level, format, args) { // 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; } } diff --git a/scripts/jest/matchers/toWarnDev.js b/scripts/jest/matchers/toWarnDev.js index 1114b65dfa..b93a4daf75 100644 --- a/scripts/jest/matchers/toWarnDev.js +++ b/scripts/jest/matchers/toWarnDev.js @@ -80,6 +80,34 @@ const createMatcherFor = (consoleMethod, matcherName) => return; } + // Append Component Stacks. Simulates a framework or DevTools appending them. + if ( + typeof format === 'string' && + (consoleMethod === 'error' || consoleMethod === 'warn') + ) { + const React = require('react'); + if (React.captureOwnerStack) { + // enableOwnerStacks enabled. When it's always on, we can assume this case. + const stack = React.captureOwnerStack(); + if (stack) { + format += '%s'; + args.push(stack); + } + } else { + // Otherwise we have to use internals to emulate parent stacks. + 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; + if (ReactSharedInternals && ReactSharedInternals.getCurrentStack) { + const stack = ReactSharedInternals.getCurrentStack(); + if (stack !== '') { + format += '%s'; + args.push(stack); + } + } + } + } + const message = util.format(format, ...args); const normalizedMessage = normalizeCodeLocInfo(message);