From cbb313253fdf96c4fca06a24ed4cbd3f3df9db80 Mon Sep 17 00:00:00 2001 From: Rick Hanlon Date: Mon, 30 Sep 2024 09:15:04 -0700 Subject: [PATCH] Fix errors with component stacks reported as warnings (#46637) Summary: Ok so this is a doozy. ## Overview There was a report that some console.error calls were being shown as warnings in LogBox but as console.error in the console. The only time we should downlevel an error to a warning is if the custom warning filter says so (which is used for some noisy legacy warning filter warnings internally). However, in when I switched from using the `Warning: ` prefix, to using the presence of component stacks, I subtly missed the default warning filter case. In the internal warning filter, the `monitorEvent` is always set to something other than `unknown` and if it's set to `warning_unhandled` then `suppressDialog_LEGACY` is always false. However, the default values for the warning filter are that `monitorEvent = 'unknown'` and `suppressDialog_LEGACY = true`. In this case, we would downlevel the error to a warning. ## What's the fix? Change the default settings for the warning filter. ## What's the root cause? Bad configuration combinations in a fragile system that needs cleaned up, and really really bad testing practices with excessive mocking and snapshot testing (I can say that, I wrote the tests) ## How could it have been caught? It was, but I turned off the integration tests while landing the component stack changes because of mismatches between flags internally and in OSS, and never turned them back on. Changelog: [General] [Fixed] - Fix logbox reporting React errors as Warnings Pull Request resolved: https://github.com/facebook/react-native/pull/46637 Reviewed By: huntie Differential Revision: D63349613 Pulled By: rickhanlonii fbshipit-source-id: 32e3fa4e2f2077114a6e9f4feac73673973ab50c --- .../Libraries/LogBox/Data/LogBoxData.js | 4 +- .../__tests__/LogBox-integration-test.js | 6 +-- .../Libraries/LogBox/__tests__/LogBox-test.js | 39 +++++++++++++++++++ 3 files changed, 44 insertions(+), 5 deletions(-) diff --git a/packages/react-native/Libraries/LogBox/Data/LogBoxData.js b/packages/react-native/Libraries/LogBox/Data/LogBoxData.js index 367a78dd70d..08c631c8fec 100644 --- a/packages/react-native/Libraries/LogBox/Data/LogBoxData.js +++ b/packages/react-native/Libraries/LogBox/Data/LogBoxData.js @@ -82,9 +82,9 @@ let warningFilter: WarningFilter = function (format) { return { finalFormat: format, forceDialogImmediately: false, - suppressDialog_LEGACY: true, + suppressDialog_LEGACY: false, suppressCompletely: false, - monitorEvent: 'unknown', + monitorEvent: 'warning_unhandled', monitorListVersion: 0, monitorSampleRate: 1, }; diff --git a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js index b6583f74a4a..1e64e5e11ef 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js +++ b/packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js @@ -87,7 +87,7 @@ describe('LogBox', () => { expect.stringMatching('at DoesNotUseKey'), ]); expect(spy).toHaveBeenCalledWith({ - level: 'warn', + level: 'error', category: expect.stringContaining( 'Warning: Each child in a list should have a unique', ), @@ -140,7 +140,7 @@ describe('LogBox', () => { expect.stringMatching('at FragmentWithProp'), ]); expect(spy).toHaveBeenCalledWith({ - level: 'warn', + level: 'error', category: expect.stringContaining('Warning: Invalid prop'), componentStack: expect.anything(), componentStackType: expect.stringMatching(/(stack|legacy)/), @@ -236,7 +236,7 @@ describe('LogBox', () => { ), ]); expect(spy).toHaveBeenCalledWith({ - level: 'warn', + level: 'error', category: expect.stringContaining('Warning: Manual console error'), componentStack: expect.anything(), componentStackType: 'stack', diff --git a/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js b/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js index b44c6131ffb..a8a40825354 100644 --- a/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js +++ b/packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js @@ -506,6 +506,45 @@ describe('LogBox', () => { 'Custom: after installing for the second time', ); }); + + it('registers errors with component stack as errors by default, when ExceptionManager is registered first', () => { + jest.spyOn(LogBoxData, 'checkWarningFilter'); + jest.spyOn(LogBoxData, 'addLog'); + + ExceptionsManager.installConsoleErrorReporter(); + LogBox.install(); + + console.error( + 'HIT\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + + expect(LogBoxData.addLog).toBeCalledWith( + expect.objectContaining({level: 'error'}), + ); + expect(LogBoxData.checkWarningFilter).toBeCalledWith( + 'HIT\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + }); + + it('registers errors with component stack as errors by default, when ExceptionManager is registered second', () => { + jest.spyOn(LogBoxData, 'checkWarningFilter'); + jest.spyOn(LogBoxData, 'addLog'); + + LogBox.install(); + ExceptionsManager.installConsoleErrorReporter(); + + console.error( + 'HIT\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + + expect(LogBoxData.addLog).toBeCalledWith( + expect.objectContaining({level: 'error'}), + ); + expect(LogBoxData.checkWarningFilter).toBeCalledWith( + 'HIT\n at Text (/path/to/Component:30:175)\n at DoesNotUseKey', + ); + }); + it('registers errors without component stack as errors by default, when ExceptionManager is registered first', () => { jest.spyOn(LogBoxData, 'checkWarningFilter'); jest.spyOn(LogBoxData, 'addException');