Skip to content

Commit 9769971

Browse files
rickhanloniifacebook-github-bot
authored andcommitted
Remove console.error patch
Summary: ## Overview This is the final boss of the new owner stacks feature. With owner stacks, we don't need to parse message strings to find the component stack for logbox. Instead, we can access the component stack directly with `captureOwnerStack`. This means we don't need to install the LogBox console.error patch and can greatly simplify the process of handling errors and make it more reliable. To do this, we rely only on adding LogBox to the ExceptionManager: - `reactConsoleErrorHandler` -> `LogBox.addConsoleLog` - `reportException` -> `LogBox.addException` [General][Fixed] - Remove LogBox patch, de-duplicating errors ## Benefits As a side effect, this removes a lot of duplicate errors. For example, currently if a component throws, you get 2 errors: {F1974436906} After this, there's just the one you expect: {F1974436908} ## Followups After this lands and doesn't need reverted for some reason, we can delete a ton of code from logbox for finding and detecting stacks from errors. Differential Revision: D68380668
1 parent 6819bed commit 9769971

File tree

2 files changed

+82
-105
lines changed

2 files changed

+82
-105
lines changed

packages/react-native/Libraries/Core/ExceptionsManager.js

Lines changed: 18 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@
1212

1313
import type {ExtendedError} from './ExtendedError';
1414
import type {ExceptionData} from './NativeExceptionsManager';
15+
import {captureOwnerStack} from 'react';
1516

1617
class SyntheticError extends Error {
1718
name: string = '';
@@ -112,11 +113,16 @@ function reportException(
112113
}
113114

114115
if (__DEV__) {
115-
const LogBox = require('../LogBox/LogBox').default;
116-
LogBox.addException({
117-
...data,
118-
isComponentError: !!e.isComponentError,
119-
});
116+
// reportToConsole is only true when coming from handleException,
117+
// and the error has not yet been logged. If it's false, then it was
118+
// reported to LogBox in reactConsoleErrorHandler, and we don't need to.
119+
if (reportToConsole) {
120+
const LogBox = require('../LogBox/LogBox').default;
121+
LogBox.addException({
122+
...data,
123+
isComponentError: !!e.isComponentError,
124+
});
125+
}
120126
} else if (isFatal || e.type !== 'warn') {
121127
const NativeExceptionsManager =
122128
require('./NativeExceptionsManager').default;
@@ -243,6 +249,13 @@ function reactConsoleErrorHandler(...args) {
243249
!global.RN$handleException ||
244250
!global.RN$handleException(error, isFatal, reportToConsole)
245251
) {
252+
if (__DEV__) {
253+
// If we're not reporting to the console in reportException,
254+
// we need to report it as a console.error here.
255+
if (!reportToConsole) {
256+
require('../LogBox/LogBox').default.addConsoleLog('error', ...args);
257+
}
258+
}
246259
reportException(
247260
/* $FlowFixMe[class-object-subtyping] added when improving typing for this
248261
* parameters */

packages/react-native/Libraries/LogBox/LogBox.js

Lines changed: 64 additions & 100 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import type {ExtendedExceptionData} from './Data/parseLogBoxLog';
1313

1414
import Platform from '../Utilities/Platform';
1515
import RCTLog from '../Utilities/RCTLog';
16-
import {hasComponentStack} from './Data/parseLogBoxLog';
1716
import * as React from 'react';
1817

1918
export type {LogData, ExtendedExceptionData, IgnorePattern};
@@ -28,6 +27,7 @@ interface ILogBox {
2827
ignoreAllLogs(?boolean): void;
2928
clearAllLogs(): void;
3029
addLog(log: LogData): void;
30+
addConsoleLog(level: 'warn' | 'error', ...args: Array<mixed>): void;
3131
addException(error: ExtendedExceptionData): void;
3232
}
3333

@@ -36,11 +36,12 @@ interface ILogBox {
3636
*/
3737
if (__DEV__) {
3838
const LogBoxData = require('./Data/LogBoxData');
39-
const {parseLogBoxLog, parseInterpolation} = require('./Data/parseLogBoxLog');
39+
const {
40+
parseLogBoxLog,
41+
parseComponentStack,
42+
} = require('./Data/parseLogBoxLog');
4043

41-
let originalConsoleError;
4244
let originalConsoleWarn;
43-
let consoleErrorImpl;
4445
let consoleWarnImpl: (...args: Array<mixed>) => void;
4546

4647
let isLogBoxInstalled: boolean = false;
@@ -70,22 +71,20 @@ if (__DEV__) {
7071
// IMPORTANT: we only overwrite `console.error` and `console.warn` once.
7172
// When we uninstall we keep the same reference and only change its
7273
// internal implementation
73-
const isFirstInstall = originalConsoleError == null;
74+
const isFirstInstall = originalConsoleWarn == null;
7475
if (isFirstInstall) {
75-
originalConsoleError = console.error.bind(console);
76+
// We only patch warning for legacy reasons.
77+
// This will be removed in the future, once warnings
78+
// are fully moved to fusebox. Error handling is done
79+
// via the ExceptionManager.
7680
originalConsoleWarn = console.warn.bind(console);
7781

78-
// $FlowExpectedError[cannot-write]
79-
console.error = (...args) => {
80-
consoleErrorImpl(...args);
81-
};
8282
// $FlowExpectedError[cannot-write]
8383
console.warn = (...args) => {
8484
consoleWarnImpl(...args);
8585
};
8686
}
8787

88-
consoleErrorImpl = registerError;
8988
consoleWarnImpl = registerWarning;
9089

9190
if (Platform.isTesting) {
@@ -108,7 +107,6 @@ if (__DEV__) {
108107
// decorated again after installing LogBox. E.g.:
109108
// Before uninstalling: original > LogBox > OtherErrorHandler
110109
// After uninstalling: original > LogBox (noop) > OtherErrorHandler
111-
consoleErrorImpl = originalConsoleError;
112110
consoleWarnImpl = originalConsoleWarn;
113111
},
114112

@@ -134,6 +132,56 @@ if (__DEV__) {
134132
}
135133
},
136134

135+
addConsoleLog(level: 'warn' | 'error', ...args: Array<mixed>) {
136+
let filteredLevel: 'warn' | 'error' | 'fatal' = level;
137+
try {
138+
let format = args[0];
139+
if (typeof format === 'string') {
140+
const filterResult =
141+
require('../LogBox/Data/LogBoxData').checkWarningFilter(format);
142+
if (filterResult.monitorEvent !== 'warning_unhandled') {
143+
if (filterResult.suppressCompletely) {
144+
return;
145+
}
146+
147+
if (filterResult.suppressDialog_LEGACY === true) {
148+
filteredLevel = 'warn';
149+
} else if (filterResult.forceDialogImmediately === true) {
150+
filteredLevel = 'fatal'; // Do not downgrade. These are real bugs with same severity as throws.
151+
}
152+
}
153+
}
154+
155+
const result = parseLogBoxLog(args);
156+
const category = result.category;
157+
const message = result.message;
158+
let componentStackType = result.componentStackType;
159+
let componentStack = result.componentStack;
160+
if (
161+
(!componentStack || componentStack.length === 0) &&
162+
// $FlowExpectedError[prop-missing]
163+
React.captureOwnerStack
164+
) {
165+
const ownerStack = React.captureOwnerStack();
166+
const parsedComponentStack = parseComponentStack(ownerStack);
167+
componentStack = parsedComponentStack.stack;
168+
componentStackType = parsedComponentStack.type;
169+
}
170+
171+
if (!LogBoxData.isMessageIgnored(message.content)) {
172+
LogBoxData.addLog({
173+
level: filteredLevel,
174+
category,
175+
message,
176+
componentStack,
177+
componentStackType,
178+
});
179+
}
180+
} catch (err) {
181+
LogBoxData.reportLogBoxError(err);
182+
}
183+
},
184+
137185
addException,
138186
};
139187

@@ -149,14 +197,9 @@ if (__DEV__) {
149197
return typeof args[0] === 'string' && args[0].startsWith('(ADVICE)');
150198
};
151199

152-
const isWarningModuleWarning = (...args: Array<mixed>) => {
153-
return typeof args[0] === 'string' && args[0].startsWith('Warning: ');
154-
};
155-
156200
const registerWarning = (...args: Array<mixed>): void => {
157201
// Let warnings within LogBox itself fall through.
158202
if (LogBoxData.isLogBoxErrorMessage(String(args[0]))) {
159-
originalConsoleError(...args);
160203
return;
161204
} else {
162205
// Be sure to pass LogBox warnings through.
@@ -182,89 +225,6 @@ if (__DEV__) {
182225
LogBoxData.reportLogBoxError(err);
183226
}
184227
};
185-
186-
/* $FlowFixMe[missing-local-annot] The type annotation(s) required by Flow's
187-
* LTI update could not be added via codemod */
188-
const registerError = (...args): void => {
189-
// Let errors within LogBox itself fall through.
190-
if (LogBoxData.isLogBoxErrorMessage(args[0])) {
191-
originalConsoleError(...args);
192-
return;
193-
}
194-
195-
try {
196-
let stack;
197-
// $FlowFixMe[prop-missing] Not added to flow types yet.
198-
if (!hasComponentStack(args) && React.captureOwnerStack != null) {
199-
stack = React.captureOwnerStack();
200-
if (!hasComponentStack(args)) {
201-
console.log('hit');
202-
if (stack !== '') {
203-
args[0] = args[0] += '%s';
204-
args.push(stack);
205-
}
206-
}
207-
}
208-
if (!isWarningModuleWarning(...args) && !hasComponentStack(args)) {
209-
// Only show LogBox for the 'warning' module, or React errors with
210-
// component stacks, otherwise pass the error through.
211-
//
212-
// By passing through, this will get picked up by the React console override,
213-
// potentially adding the component stack. React then passes it back to the
214-
// React Native ExceptionsManager, which reports it to LogBox as an error.
215-
//
216-
// Ideally, we refactor all RN error handling so that LogBox patching
217-
// errors is not necessary, and they are reported the same as a framework.
218-
// The blocker to this is that the ExceptionManager console.error override
219-
// strigifys all of the args before passing it through to LogBox, which
220-
// would lose all of the interpolation information.
221-
//
222-
// The 'warning' module needs to be handled here because React internally calls
223-
// `console.error('Warning: ')` with the component stack already included.
224-
originalConsoleError(...args);
225-
return;
226-
}
227-
228-
let format = args[0].replace('Warning: ', '');
229-
const filterResult = LogBoxData.checkWarningFilter(format);
230-
let level = 'error';
231-
if (filterResult.monitorEvent !== 'warning_unhandled') {
232-
if (filterResult.suppressCompletely) {
233-
return;
234-
}
235-
236-
if (filterResult.suppressDialog_LEGACY === true) {
237-
level = 'warn';
238-
} else if (filterResult.forceDialogImmediately === true) {
239-
level = 'fatal'; // Do not downgrade. These are real bugs with same severity as throws.
240-
}
241-
}
242-
243-
// Unfortunately, we need to add the Warning: prefix back for downstream dependencies.
244-
// Downstream, we check for this prefix to know that LogBox already handled it, so
245-
// it doesn't get reported back to LogBox. It's an absolute mess.
246-
args[0] = `Warning: ${filterResult.finalFormat}`;
247-
const {category, message, componentStack, componentStackType} =
248-
parseLogBoxLog(args);
249-
250-
// Interpolate the message so they are formatted for adb and other CLIs.
251-
// This is different than the message.content above because it includes component stacks.
252-
const interpolated = parseInterpolation(args);
253-
originalConsoleError(interpolated.message.content);
254-
255-
if (!LogBoxData.isMessageIgnored(message.content)) {
256-
LogBoxData.addLog({
257-
level,
258-
category,
259-
message,
260-
componentStack,
261-
componentStackType,
262-
});
263-
}
264-
} catch (err) {
265-
LogBoxData.reportLogBoxError(err);
266-
}
267-
};
268228
} else {
269229
LogBox = {
270230
install(): void {
@@ -295,6 +255,10 @@ if (__DEV__) {
295255
// Do nothing.
296256
},
297257

258+
addConsoleLog(level: 'warn' | 'error', ...args: Array<mixed>): void {
259+
// Do nothing.
260+
},
261+
298262
addException(error: ExtendedExceptionData): void {
299263
// Do nothing.
300264
},

0 commit comments

Comments
 (0)