Skip to content

Commit e0e64e5

Browse files
rickhanloniifacebook-github-bot
rickhanlonii
authored and
facebook-github-bot
committed
Do not passthrough logbox errors that already have a component stack
Summary: In facebook/react#29839 we removed the `Warning: ` prefix. This PR replaces the special cases in LogBox for `Warning: ` to use the presence of a component stack instead. This is what LogBox really cares about anyway, since the reason to let errors pass through to the exception manager is to let DevTools add the component stacks. Changelog: [General] [Fix] Fix logbox reporting for React errors Differential Revision: D58441017
1 parent bfb3b70 commit e0e64e5

File tree

6 files changed

+174
-47
lines changed

6 files changed

+174
-47
lines changed

‎packages/react-native/Libraries/LogBox/Data/LogBoxData.js

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ import type {LogLevel} from './LogBoxLog';
1515
import type {
1616
Category,
1717
ComponentStack,
18+
ComponentStackType,
1819
ExtendedExceptionData,
1920
Message,
2021
} from './parseLogBoxLog';
@@ -30,6 +31,7 @@ export type LogData = $ReadOnly<{|
3031
message: Message,
3132
category: Category,
3233
componentStack: ComponentStack,
34+
componentStackType?: ComponentStackType,
3335
stack?: string,
3436
|}>;
3537

@@ -209,6 +211,7 @@ export function addLog(log: LogData): void {
209211
stack,
210212
category: log.category,
211213
componentStack: log.componentStack,
214+
componentStackType: log.componentStackType,
212215
}),
213216
);
214217
} catch (error) {

‎packages/react-native/Libraries/LogBox/Data/__tests__/parseLogBoxLog-test.js

Lines changed: 36 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ describe('parseLogBoxLog', () => {
115115
it('does not duplicate message if component stack found but not parsed', () => {
116116
expect(
117117
parseLogBoxLog([
118-
'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s',
118+
'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s',
119119
'\n\nCheck the render method of `MyOtherComponent`.',
120120
'',
121121
'\n in\n in\n in',
@@ -124,18 +124,18 @@ describe('parseLogBoxLog', () => {
124124
componentStackType: 'legacy',
125125
componentStack: [],
126126
category:
127-
'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.',
127+
'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.',
128128
message: {
129129
content:
130-
'Warning: Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.',
130+
'Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.',
131131
substitutions: [
132132
{
133133
length: 48,
134-
offset: 62,
134+
offset: 53,
135135
},
136136
{
137137
length: 0,
138-
offset: 110,
138+
offset: 101,
139139
},
140140
],
141141
},
@@ -145,7 +145,7 @@ describe('parseLogBoxLog', () => {
145145
it('detects a component stack in an interpolated warning', () => {
146146
expect(
147147
parseLogBoxLog([
148-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s',
148+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s',
149149
'\n\nCheck the render method of `Container(Component)`.',
150150
'\n in MyComponent (at filename.js:1)\n in MyOtherComponent (at filename2.js:1)',
151151
]),
@@ -164,14 +164,14 @@ describe('parseLogBoxLog', () => {
164164
},
165165
],
166166
category:
167-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s',
167+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s',
168168
message: {
169169
content:
170-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `Container(Component)`.',
170+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `Container(Component)`.',
171171
substitutions: [
172172
{
173173
length: 52,
174-
offset: 129,
174+
offset: 120,
175175
},
176176
],
177177
},
@@ -801,7 +801,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
801801
it('detects a component stack in an interpolated warning', () => {
802802
expect(
803803
parseLogBoxLog([
804-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s',
804+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s',
805805
'\n\nCheck the render method of `MyComponent`.',
806806
'\n in MyComponent (created by MyOtherComponent)\n in MyOtherComponent (created by MyComponent)\n in MyAppComponent (created by MyOtherComponent)',
807807
]),
@@ -825,14 +825,14 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
825825
},
826826
],
827827
category:
828-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s',
828+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s',
829829
message: {
830830
content:
831-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.',
831+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.',
832832
substitutions: [
833833
{
834834
length: 43,
835-
offset: 129,
835+
offset: 120,
836836
},
837837
],
838838
},
@@ -907,7 +907,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
907907
it('detects a component stack in the nth argument', () => {
908908
expect(
909909
parseLogBoxLog([
910-
'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s',
910+
'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s',
911911
'\n\nCheck the render method of `MyOtherComponent`.',
912912
'',
913913
'\n in MyComponent (created by MyOtherComponent)\n in MyOtherComponent (created by MyComponent)\n in MyAppComponent (created by MyOtherComponent)',
@@ -932,18 +932,18 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
932932
},
933933
],
934934
category:
935-
'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.',
935+
'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.',
936936
message: {
937937
content:
938-
'Warning: Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.',
938+
'Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.',
939939
substitutions: [
940940
{
941941
length: 48,
942-
offset: 62,
942+
offset: 53,
943943
},
944944
{
945945
length: 0,
946-
offset: 110,
946+
offset: 101,
947947
},
948948
],
949949
},
@@ -1076,7 +1076,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
10761076
it('detects a component stack in an interpolated warning', () => {
10771077
expect(
10781078
parseLogBoxLog([
1079-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s',
1079+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s',
10801080
'\n\nCheck the render method of `MyComponent`.',
10811081
'\n at MyComponent (/path/to/filename.js:1:2)\n at MyOtherComponent\n at MyAppComponent (/path/to/app.js:100:20)',
10821082
]),
@@ -1099,14 +1099,14 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
10991099
},
11001100
],
11011101
category:
1102-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s',
1102+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s',
11031103
message: {
11041104
content:
1105-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.',
1105+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.',
11061106
substitutions: [
11071107
{
11081108
length: 43,
1109-
offset: 129,
1109+
offset: 120,
11101110
},
11111111
],
11121112
},
@@ -1179,7 +1179,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
11791179
it('detects a component stack in the nth argument', () => {
11801180
expect(
11811181
parseLogBoxLog([
1182-
'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s',
1182+
'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s',
11831183
'\n\nCheck the render method of `MyOtherComponent`.',
11841184
'',
11851185
'\n at MyComponent (/path/to/filename.js:1:2)\n at MyOtherComponent\n at MyAppComponent (/path/to/app.js:100:20)',
@@ -1203,18 +1203,18 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
12031203
},
12041204
],
12051205
category:
1206-
'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.',
1206+
'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.',
12071207
message: {
12081208
content:
1209-
'Warning: Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.',
1209+
'Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.',
12101210
substitutions: [
12111211
{
12121212
length: 48,
1213-
offset: 62,
1213+
offset: 53,
12141214
},
12151215
{
12161216
length: 0,
1217-
offset: 110,
1217+
offset: 101,
12181218
},
12191219
],
12201220
},
@@ -1285,7 +1285,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
12851285
it('detects a component stack in an interpolated warning', () => {
12861286
expect(
12871287
parseLogBoxLog([
1288-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s',
1288+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s%s',
12891289
'\n\nCheck the render method of `MyComponent`.',
12901290
'\nMyComponent@/path/to/filename.js:1:2\nforEach@[native code]\nMyAppComponent@/path/to/app.js:100:20',
12911291
]),
@@ -1312,14 +1312,14 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
13121312
},
13131313
],
13141314
category:
1315-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s',
1315+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?%s',
13161316
message: {
13171317
content:
1318-
'Warning: Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.',
1318+
'Function components cannot be given refs. Attempts to access this ref will fail. Did you mean to use React.forwardRef()?\n\nCheck the render method of `MyComponent`.',
13191319
substitutions: [
13201320
{
13211321
length: 43,
1322-
offset: 129,
1322+
offset: 120,
13231323
},
13241324
],
13251325
},
@@ -1484,7 +1484,7 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
14841484
it('detects a component stack in the nth argument', () => {
14851485
expect(
14861486
parseLogBoxLog([
1487-
'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s',
1487+
'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.%s',
14881488
'\n\nCheck the render method of `MyOtherComponent`.',
14891489
'',
14901490
'\nMyComponent@/path/to/filename.js:1:2\nforEach@[native code]\nMyAppComponent@/path/to/app.js:100:20',
@@ -1512,18 +1512,18 @@ Please follow the instructions at: fburl.com/rn-remote-assets`,
15121512
},
15131513
],
15141514
category:
1515-
'Warning: Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.',
1515+
'Each child in a list should have a unique "key" prop.%s%s See https://fb.me/react-warning-keys for more information.',
15161516
message: {
15171517
content:
1518-
'Warning: Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.',
1518+
'Each child in a list should have a unique "key" prop.\n\nCheck the render method of `MyOtherComponent`. See https://fb.me/react-warning-keys for more information.',
15191519
substitutions: [
15201520
{
15211521
length: 48,
1522-
offset: 62,
1522+
offset: 53,
15231523
},
15241524
{
15251525
length: 0,
1526-
offset: 110,
1526+
offset: 101,
15271527
},
15281528
],
15291529
},

‎packages/react-native/Libraries/LogBox/Data/parseLogBoxLog.js

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -93,6 +93,15 @@ const RE_BABEL_CODE_FRAME_MARKER_PATTERN = new RegExp(
9393
'm',
9494
);
9595

96+
export function hasComponentStack(args: $ReadOnlyArray<mixed>): boolean {
97+
for (const arg of args) {
98+
if (typeof arg === 'string' && isComponentStack(arg)) {
99+
return true;
100+
}
101+
}
102+
return false;
103+
}
104+
96105
export type ExtendedExceptionData = ExceptionData & {
97106
isComponentError: boolean,
98107
...

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

Lines changed: 26 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ 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';
1617

1718
export type {LogData, ExtendedExceptionData, IgnorePattern};
1819

@@ -176,12 +177,20 @@ if (__DEV__) {
176177
}
177178

178179
try {
179-
if (!isWarningModuleWarning(...args)) {
180-
// Only show LogBox for the 'warning' module, otherwise pass through.
180+
if (!isWarningModuleWarning(...args) && !hasComponentStack(args)) {
181+
// Only show LogBox for the 'warning' modulee, or React errors with
182+
// component stacks, otherwise pass the error through.
183+
//
181184
// By passing through, this will get picked up by the React console override,
182185
// potentially adding the component stack. React then passes it back to the
183186
// React Native ExceptionsManager, which reports it to LogBox as an error.
184187
//
188+
// Ideally, we refactor all RN error handling so that LogBox patching
189+
// errors is not necessary, and they are reported the same as a framework.
190+
// The blocker to this is that the ExceptionManager console.error override
191+
// strigifys all of the args before passing it through to LogBox, which
192+
// would lose all of the interpolation information.
193+
//
185194
// The 'warning' module needs to be handled here because React internally calls
186195
// `console.error('Warning: ')` with the component stack already included.
187196
originalConsoleError(...args);
@@ -190,20 +199,25 @@ if (__DEV__) {
190199

191200
const format = args[0].replace('Warning: ', '');
192201
const filterResult = LogBoxData.checkWarningFilter(format);
193-
if (filterResult.suppressCompletely) {
194-
return;
195-
}
196-
197202
let level = 'error';
198-
if (filterResult.suppressDialog_LEGACY === true) {
199-
level = 'warn';
200-
} else if (filterResult.forceDialogImmediately === true) {
201-
level = 'fatal'; // Do not downgrade. These are real bugs with same severity as throws.
203+
if (filterResult.monitorEvent !== 'warning_unhandled') {
204+
if (filterResult.suppressCompletely) {
205+
return;
206+
}
207+
208+
if (filterResult.suppressDialog_LEGACY === true) {
209+
level = 'warn';
210+
} else if (filterResult.forceDialogImmediately === true) {
211+
level = 'fatal'; // Do not downgrade. These are real bugs with same severity as throws.
212+
}
202213
}
203214

204215
// Unfortunately, we need to add the Warning: prefix back for downstream dependencies.
216+
// Downstream, we check for this prefix to know that LogBox already handled it, so
217+
// it doesn't get reported back to LogBox. It's an absolute mess.
205218
args[0] = `Warning: ${filterResult.finalFormat}`;
206-
const {category, message, componentStack} = parseLogBoxLog(args);
219+
const {category, message, componentStack, componentStackType} =
220+
parseLogBoxLog(args);
207221

208222
// Interpolate the message so they are formatted for adb and other CLIs.
209223
// This is different than the message.content above because it includes component stacks.
@@ -216,6 +230,7 @@ if (__DEV__) {
216230
category,
217231
message,
218232
componentStack,
233+
componentStackType,
219234
});
220235
}
221236
} catch (err) {

‎packages/react-native/Libraries/LogBox/__tests__/LogBox-integration-test.js

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,8 @@ describe.skip('LogBox', () => {
9595
expect(mockError.mock.calls[0].map(cleanPath)).toMatchSnapshot(
9696
'Log passed to console error',
9797
);
98+
99+
// The Warning: prefix is added due to a hack in LogBox to prevent double logging.
98100
expect(mockError.mock.calls[0][0].startsWith('Warning: ')).toBe(true);
99101
});
100102

@@ -123,6 +125,8 @@ describe.skip('LogBox', () => {
123125
expect(mockError.mock.calls[0].map(cleanPath)).toMatchSnapshot(
124126
'Log passed to console error',
125127
);
128+
129+
// The Warning: prefix is added due to a hack in LogBox to prevent double logging.
126130
expect(mockError.mock.calls[0][0].startsWith('Warning: ')).toBe(true);
127131
});
128132
});

0 commit comments

Comments
 (0)