Skip to content

Commit

Permalink
Fix errors with component stacks reported as warnings (#46637)
Browse files Browse the repository at this point in the history
Summary:
Pull Request resolved: #46637

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

Reviewed By: huntie

Differential Revision: D63349613
  • Loading branch information
rickhanlonii authored and facebook-github-bot committed Sep 26, 2024
1 parent 53b7151 commit ec14c17
Show file tree
Hide file tree
Showing 3 changed files with 44 additions and 5 deletions.
4 changes: 2 additions & 2 deletions packages/react-native/Libraries/LogBox/Data/LogBoxData.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
};
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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',
),
Expand Down Expand Up @@ -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)/),
Expand Down Expand Up @@ -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',
Expand Down
39 changes: 39 additions & 0 deletions packages/react-native/Libraries/LogBox/__tests__/LogBox-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down

0 comments on commit ec14c17

Please sign in to comment.