@samitouri / QOS-React-2 / commits / 2e540e22b2

Set the current fiber to the source of the error during error reporting (#29044)

This lets us expose the component stack to the error reporting that happens here as `console.error` patching. Now if you just call `console.error` in the error handlers it'll get the component stack added to the end by React DevTools. However, unfortunately this happens a little too late so the Fiber will be disconnected with its `.return` pointer set to null already. So it'll be too late to extract a parent component stack from but you can at least get the stack from source to error boundary. To work around this I manually add the parent component stack in our default handlers when owner stacks are off. We could potentially fix this but you can also just include it yourself if you're calling `console.error` and it's not a problem for owner stacks. This is not a problem for owner stacks because we'll still have those and so for those just calling `console.error` just works. However, the main feature is that by letting React add them, we can switch to using native error stacks when available.

Sebastian Markbåge committed May 23, 2024 at 12:39 UTC 2e540e22b2b4038a278b2875306976b016fb31a9

12 files changed +147 -86

packages/react-devtools-shared/src/__tests__/treeContext-test.js

+9 -13

@@ -2586,16 +2586,14 @@ describe('TreeListContext', () => {

2586 utils.act(() => TestRenderer.create(<Contexts />));

2587

2588 expect(store).toMatchInlineSnapshot(`

2589 - ✕ 1, ⚠ 0

2589 [root]

2591 - <ErrorBoundary> ✕

2590 + <ErrorBoundary>

2591 `);

2592

2593 selectNextErrorOrWarning();

2594 expect(state).toMatchInlineSnapshot(`

2596 - ✕ 1, ⚠ 0

2595 [root]

2598 - → <ErrorBoundary> ✕

2596 + <ErrorBoundary>

2597 `);

2598

2599 utils.act(() => unmount());

@@ -2648,16 +2646,14 @@ describe('TreeListContext', () => {

2646 utils.act(() => TestRenderer.create(<Contexts />));

2647

2648 expect(store).toMatchInlineSnapshot(`

2651 - ✕ 1, ⚠ 0

2649 [root]

2653 - <ErrorBoundary> ✕

2650 + <ErrorBoundary>

2651 `);

2652

2653 selectNextErrorOrWarning();

2654 expect(state).toMatchInlineSnapshot(`

2658 - ✕ 1, ⚠ 0

2655 [root]

2660 - → <ErrorBoundary> ✕

2656 + <ErrorBoundary>

2657 `);

2658

2659 utils.act(() => unmount());

@@ -2705,18 +2701,18 @@ describe('TreeListContext', () => {

2701 utils.act(() => TestRenderer.create(<Contexts />));

2702

2703 expect(store).toMatchInlineSnapshot(`

2708 - ✕ 2, ⚠ 0

2704 + ✕ 1, ⚠ 0

2705 [root]

2710 - ▾ <ErrorBoundary> ✕

2706 + ▾ <ErrorBoundary>

2707 <Child> ✕

2708 `);

2709

2710 selectNextErrorOrWarning();

2711 expect(state).toMatchInlineSnapshot(`

2716 - ✕ 2, ⚠ 0

2712 + ✕ 1, ⚠ 0

2713 [root]

2718 - → ▾ <ErrorBoundary> ✕

2719 - <Child> ✕

2714 + ▾ <ErrorBoundary>

2715 + → <Child> ✕

2716 `);

2717 });

2718 });

packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js

+6 -6

@@ -142,8 +142,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

142 // Addendum by React:

143 expect.stringContaining('%s'),

144 expect.stringContaining('An error occurred in the <Foo> component'),

145 - expect.stringContaining('Foo'),

145 expect.stringContaining('Consider adding an error boundary'),

146 + expect.stringContaining('Foo'),

147 ],

148 ]);

149 } else {

@@ -207,8 +207,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

207 expect.stringContaining(

208 'The above error occurred in the <Foo> component',

209 ),

210 - expect.stringContaining('Foo'),

210 expect.stringContaining('ErrorBoundary'),

211 + expect.stringContaining('Foo'),

212 ],

213 ]);

214 } else {

@@ -273,8 +273,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

273 // Addendum by React:

274 expect.stringContaining('%s'),

275 expect.stringContaining('An error occurred in the <Foo> component'),

276 - expect.stringContaining('Foo'),

276 expect.stringContaining('Consider adding an error boundary'),

277 + expect.stringContaining('Foo'),

278 ],

279 ]);

280 } else {

@@ -343,8 +343,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

343 expect.stringContaining(

344 'The above error occurred in the <Foo> component',

345 ),

346 - expect.stringContaining('Foo'),

346 expect.stringContaining('ErrorBoundary'),

347 + expect.stringContaining('Foo'),

348 ],

349 ]);

350 } else {

@@ -409,8 +409,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

409 // Addendum by React:

410 expect.stringContaining('%s'),

411 expect.stringContaining('An error occurred in the <Foo> component'),

412 - expect.stringContaining('Foo'),

412 expect.stringContaining('Consider adding an error boundary'),

413 + expect.stringContaining('Foo'),

414 ],

415 ]);

416 } else {

@@ -477,8 +477,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

477 expect.stringContaining(

478 'The above error occurred in the <Foo> component',

479 ),

480 - expect.stringContaining('Foo'),

480 expect.stringContaining('ErrorBoundary'),

481 + expect.stringContaining('Foo'),

482 ],

483 ]);

484 } else {

packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js

+7 -9

@@ -161,8 +161,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

161 expect.stringContaining('%s'),

162 // Addendum by React:

163 expect.stringContaining('An error occurred in the <Foo> component'),

164 - expect.stringContaining('Foo'),

164 expect.stringContaining('Consider adding an error boundary'),

165 + expect.stringContaining('Foo'),

166 ],

167 ]);

168

@@ -238,8 +238,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

238 expect.stringContaining(

239 'The above error occurred in the <Foo> component',

240 ),

241 - expect.stringContaining('Foo'),

241 expect.stringContaining('ErrorBoundary'),

242 + expect.stringContaining('Foo'),

243 ],

244 ]);

245 } else {

@@ -307,11 +307,9 @@ describe('ReactDOMConsoleErrorReporting', () => {

307 expect.stringContaining('%s'),

308

309 // Addendum by React:

310 - expect.stringContaining(

311 - 'An error occurred in the <Foo> component:',

312 - ),

313 - expect.stringContaining('Foo'),

310 + expect.stringContaining('An error occurred in the <Foo> component'),

311 expect.stringContaining('Consider adding an error boundary'),

312 + expect.stringContaining('Foo'),

313 ],

314 ]);

315

@@ -391,8 +389,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

389 expect.stringContaining(

390 'The above error occurred in the <Foo> component',

391 ),

394 - expect.stringContaining('Foo'),

392 expect.stringContaining('ErrorBoundary'),

393 + expect.stringContaining('Foo'),

394 ],

395 ]);

396 } else {

@@ -461,8 +459,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

459

460 // Addendum by React:

461 expect.stringContaining('An error occurred in the <Foo> component'),

464 - expect.stringContaining('Foo'),

462 expect.stringContaining('Consider adding an error boundary'),

463 + expect.stringContaining('Foo'),

464 ],

465 ]);

466

@@ -541,8 +539,8 @@ describe('ReactDOMConsoleErrorReporting', () => {

539 expect.stringContaining(

540 'The above error occurred in the <Foo> component',

541 ),

544 - expect.stringContaining('Foo'),

542 expect.stringContaining('ErrorBoundary'),

543 + expect.stringContaining('Foo'),

544 ],

545 ]);

546 } else {

packages/react-dom/src/__tests__/ReactErrorBoundaries-test.internal.js

+1 -1

@@ -727,7 +727,7 @@ describe('ReactErrorBoundaries', () => {

727 if (__DEV__) {

728 expect(console.error).toHaveBeenCalledTimes(1);

729 expect(console.error.mock.calls[0][2]).toContain(

730 - 'The above error occurred in the <BrokenRender> component:',

730 + 'The above error occurred in the <BrokenRender> component',

731 );

732 }

733

packages/react-dom/src/__tests__/ReactLegacyErrorBoundaries-test.internal.js

+1 -1

@@ -705,7 +705,7 @@ describe('ReactLegacyErrorBoundaries', () => {

705 'ReactDOM.render has not been supported since React 18',

706 );

707 expect(console.error.mock.calls[1][2]).toContain(

708 - 'The above error occurred in the <BrokenRender> component:',

708 + 'The above error occurred in the <BrokenRender> component',

709 );

710 }

711

packages/react-reconciler/src/ReactFiberErrorLogger.js

+61 -27

@@ -18,6 +18,8 @@ import reportGlobalError from 'shared/reportGlobalError';

19 import ReactSharedInternals from 'shared/ReactSharedInternals';

21 +import {enableOwnerStacks} from 'shared/ReactFeatureFlags';

22 +

23 // Side-channel since I'm not sure we want to make this part of the public API

24 let componentName: null | string = null;

25 let errorBoundaryName: null | string = null;

@@ -34,20 +36,36 @@ export function defaultOnUncaughtError(

36 // So we add those into a separate console.warn.

37 reportGlobalError(error);

38 if (__DEV__) {

37 - const componentStack =

38 - errorInfo.componentStack != null ? errorInfo.componentStack : '';

39 -

39 const componentNameMessage = componentName

41 - ? `An error occurred in the <${componentName}> component:`

42 - : 'An error occurred in one of your React components:';

40 + ? `An error occurred in the <${componentName}> component.`

41 + : 'An error occurred in one of your React components.';

44 - console['warn'](

45 - '%s\n%s\n\n%s',

46 - componentNameMessage,

47 - componentStack || '',

43 + const errorBoundaryMessage =

44 'Consider adding an error boundary to your tree to customize error handling behavior.\n' +

49 - 'Visit https://react.dev/link/error-boundaries to learn more about error boundaries.',

50 - );

45 + 'Visit https://react.dev/link/error-boundaries to learn more about error boundaries.';

46 +

47 + if (enableOwnerStacks) {

48 + console.warn(

49 + '%s\n\n%s\n',

50 + componentNameMessage,

51 + errorBoundaryMessage,

52 + // We let our console.error wrapper add the component stack to the end.

53 + );

54 + } else {

55 + // The current Fiber is disconnected at this point which means that console printing

56 + // cannot add a component stack since it terminates at the deletion node. This is not

57 + // a problem for owner stacks which are not disconnected but for the parent component

58 + // stacks we need to use the snapshot we've previously extracted.

59 + const componentStack =

60 + errorInfo.componentStack != null ? errorInfo.componentStack : '';

61 + // Don't transform to our wrapper

62 + console['warn'](

63 + '%s\n\n%s\n%s',

64 + componentNameMessage,

65 + errorBoundaryMessage,

66 + componentStack,

67 + );

68 + }

69 }

70 }

@@ -63,31 +81,47 @@ export function defaultOnCaughtError(

82 // Caught by error boundary

83 if (__DEV__) {

66 - const componentStack =

67 - errorInfo.componentStack != null ? errorInfo.componentStack : '';

68 -

84 const componentNameMessage = componentName

70 - ? `The above error occurred in the <${componentName}> component:`

71 - : 'The above error occurred in one of your React components:';

85 + ? `The above error occurred in the <${componentName}> component.`

86 + : 'The above error occurred in one of your React components.';

88 // In development, we provide our own message which includes the component stack

89 // in addition to the error.

75 - // Don't transform to our wrapper

76 - console['error'](

77 - '%o\n\n%s\n%s\n\n%s',

78 - error,

79 - componentNameMessage,

80 - componentStack,

90 + const recreateMessage =

91 `React will try to recreate this component tree from scratch ` +

82 - `using the error boundary you provided, ${

83 - errorBoundaryName || 'Anonymous'

84 - }.`,

85 - );

92 + `using the error boundary you provided, ${

93 + errorBoundaryName || 'Anonymous'

94 + }.`;

95 +

96 + if (enableOwnerStacks) {

97 + console.error(

98 + '%o\n\n%s\n\n%s\n',

99 + error,

100 + componentNameMessage,

101 + recreateMessage,

102 + // We let our consoleWithStackDev wrapper add the component stack to the end.

103 + );

104 + } else {

105 + // The current Fiber is disconnected at this point which means that console printing

106 + // cannot add a component stack since it terminates at the deletion node. This is not

107 + // a problem for owner stacks which are not disconnected but for the parent component

108 + // stacks we need to use the snapshot we've previously extracted.

109 + const componentStack =

110 + errorInfo.componentStack != null ? errorInfo.componentStack : '';

111 + // Don't transform to our wrapper

112 + console['error'](

113 + '%o\n\n%s\n\n%s\n%s',

114 + error,

115 + componentNameMessage,

116 + recreateMessage,

117 + componentStack,

118 + );

119 + }

120 } else {

121 // In production, we print the error directly.

122 // This will include the message, the JS stack, and anything the browser wants to show.

123 // We pass the error object instead of custom message so that the browser displays the error natively.

90 - console['error'](error); // Don't transform to our wrapper

124 + console['error'](error); // Don't transform to our wrapper, however, React DevTools can still add a stack.

125 }

126 }

127

packages/react-reconciler/src/ReactFiberThrow.js

+13

@@ -87,6 +87,10 @@ import {

87 import {ConcurrentRoot} from './ReactRootTags';

88 import {noopSuspenseyCommitThenable} from './ReactFiberThenable';

89 import {REACT_POSTPONE_TYPE} from 'shared/ReactSymbols';

90 +import {

91 + setCurrentDebugFiberInDEV,

92 + getCurrentFiber as getCurrentDebugFiberInDEV,

93 +} from './ReactCurrentFiber';

95 function createRootErrorUpdate(

96 root: FiberRoot,

@@ -100,7 +104,10 @@ function createRootErrorUpdate(

104 // being called "element".

105 update.payload = {element: null};

106 update.callback = () => {

107 + const prevFiber = getCurrentDebugFiberInDEV(); // should just be the root

108 + setCurrentDebugFiberInDEV(errorInfo.source);

109 logUncaughtError(root, errorInfo);

110 + setCurrentDebugFiberInDEV(prevFiber);

111 };

112 return update;

113 }

@@ -127,7 +134,10 @@ function initializeClassErrorUpdate(

134 if (__DEV__) {

135 markFailedErrorBoundaryForHotReloading(fiber);

136 }

137 + const prevFiber = getCurrentDebugFiberInDEV(); // should be the error boundary

138 + setCurrentDebugFiberInDEV(errorInfo.source);

139 logCaughtError(root, fiber, errorInfo);

140 + setCurrentDebugFiberInDEV(prevFiber);

141 };

142 }

143

@@ -138,7 +148,10 @@ function initializeClassErrorUpdate(

148 if (__DEV__) {

149 markFailedErrorBoundaryForHotReloading(fiber);

150 }

151 + const prevFiber = getCurrentDebugFiberInDEV(); // should be the error boundary

152 + setCurrentDebugFiberInDEV(errorInfo.source);

153 logCaughtError(root, fiber, errorInfo);

154 + setCurrentDebugFiberInDEV(prevFiber);

155 if (typeof getDerivedStateFromError !== 'function') {

156 // To preserve the preexisting retry behavior of error boundaries,

157 // we keep track of which ones already failed during this batch.

packages/react-reconciler/src/ReactFiberWorkLoop.js

@@ -3038,7 +3038,9 @@ function commitRootImpl(

3038 for (let i = 0; i < recoverableErrors.length; i++) {

3039 const recoverableError = recoverableErrors[i];

3040 const errorInfo = makeErrorInfo(recoverableError.stack);

3041 + setCurrentDebugFiberInDEV(recoverableError.source);

3042 onRecoverableError(recoverableError.value, errorInfo);

3043 + resetCurrentDebugFiberInDEV();

3044 }

3045 }

3046

packages/react-reconciler/src/__tests__/ReactIncrementalErrorHandling-test.internal.js

+1 -1

@@ -1512,7 +1512,7 @@ describe('ReactIncrementalErrorHandling', () => {

1512 expect(console.error).toHaveBeenCalledTimes(1);

1513 expect(console.error.mock.calls[0][1]).toBe(notAnError);

1514 expect(console.error.mock.calls[0][2]).toContain(

1515 - 'The above error occurred in the <BadRender> component:',

1515 + 'The above error occurred in the <BadRender> component',

1516 );

1517 } else {

1518 expect(console.error).toHaveBeenCalledTimes(1);

packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js

+25 -21

@@ -85,7 +85,11 @@ describe('ReactIncrementalErrorLogging', () => {

85 expect(console.warn).toHaveBeenCalledWith(

86 expect.stringContaining('%s'),

87 expect.stringContaining(

88 - 'An error occurred in the <ErrorThrowingComponent> component:',

88 + 'An error occurred in the <ErrorThrowingComponent> component.',

89 + ),

90 + expect.stringContaining(

91 + 'Consider adding an error boundary to your tree ' +

92 + 'to customize error handling behavior.',

93 ),

94 expect.stringMatching(

95 new RegExp(

@@ -94,10 +98,6 @@ describe('ReactIncrementalErrorLogging', () => {

98 '\\s+(in|at) div(.*)',

99 ),

100 ),

97 - expect.stringContaining(

98 - 'Consider adding an error boundary to your tree ' +

99 - 'to customize error handling behavior.',

100 - ),

101 );

102 }

103 });

@@ -131,7 +131,11 @@ describe('ReactIncrementalErrorLogging', () => {

131 expect(console.warn).toHaveBeenCalledWith(

132 expect.stringContaining('%s'),

133 expect.stringContaining(

134 - 'An error occurred in the <ErrorThrowingComponent> component:',

134 + 'An error occurred in the <ErrorThrowingComponent> component.',

135 + ),

136 + expect.stringContaining(

137 + 'Consider adding an error boundary to your tree ' +

138 + 'to customize error handling behavior.',

139 ),

140 expect.stringMatching(

141 new RegExp(

@@ -140,10 +144,6 @@ describe('ReactIncrementalErrorLogging', () => {

144 '\\s+(in|at) div(.*)',

145 ),

146 ),

143 - expect.stringContaining(

144 - 'Consider adding an error boundary to your tree ' +

145 - 'to customize error handling behavior.',

146 - ),

147 );

148 }

149 });

@@ -189,7 +189,11 @@ describe('ReactIncrementalErrorLogging', () => {

189 message: 'render error',

190 }),

191 expect.stringContaining(

192 - 'The above error occurred in the <ErrorThrowingComponent> component:',

192 + 'The above error occurred in the <ErrorThrowingComponent> component.',

193 + ),

194 + expect.stringContaining(

195 + 'React will try to recreate this component tree from scratch ' +

196 + 'using the error boundary you provided, ErrorBoundary.',

197 ),

198 expect.stringMatching(

199 new RegExp(

@@ -199,10 +203,6 @@ describe('ReactIncrementalErrorLogging', () => {

203 '\\s+(in|at) div(.*)',

204 ),

205 ),

202 - expect.stringContaining(

203 - 'React will try to recreate this component tree from scratch ' +

204 - 'using the error boundary you provided, ErrorBoundary.',

205 - ),

206 );

207 } else {

208 expect(logCapturedErrorCalls[0]).toEqual(

@@ -270,17 +270,21 @@ describe('ReactIncrementalErrorLogging', () => {

270 message: 'oops',

271 }),

272 expect.stringContaining(

273 - 'The above error occurred in the <Foo> component:',

274 - ),

275 - expect.stringMatching(

276 - new RegExp(

277 - '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)',

278 - ),

273 + 'The above error occurred in the <Foo> component.',

274 ),

275 expect.stringContaining(

276 'React will try to recreate this component tree from scratch ' +

277 'using the error boundary you provided, ErrorBoundary.',

278 ),

279 + expect.stringMatching(

280 + gate(flag => flag.enableOwnerStacks)

281 + ? // With owner stacks the return path is cut off but in this case

282 + // this is also what the owner stack looks like.

283 + new RegExp('\\s+(in|at) Foo (.*)')

284 + : new RegExp(

285 + '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)',

286 + ),

287 + ),

288 );

289 } else {

290 expect(console.error).toHaveBeenCalledWith(

packages/shared/consoleWithStackDev.js

+16 -5

@@ -40,19 +40,30 @@ function printWarning(level, format, args) {

40 // When changing this logic, you might want to also

41 // update consoleWithStackDev.www.js as well.

42 if (__DEV__) {

43 + const isErrorLogger =

44 + format === '%s\n\n%s\n' || format === '%o\n\n%s\n\n%s\n';

45 +

46 const stack = ReactSharedInternals.getStackAddendum();

47 if (stack !== '') {

48 format += '%s';

49 args = args.concat([stack]);

50 }

49 - // eslint-disable-next-line react-internal/safe-string-coercion

50 - const argsWithFormat = args.map(item => String(item));

51 - // Careful: RN currently depends on this prefix

52 - argsWithFormat.unshift('Warning: ' + format);

52 + if (isErrorLogger) {

53 + // Don't prefix our default logging formatting in ReactFiberErrorLoggger.

54 + // Don't toString the arguments.

55 + args.unshift(format);

56 + } else {

57 + // TODO: Remove this prefix and stop toStringing in the wrapper and

58 + // instead do it at each callsite as needed.

59 + // Careful: RN currently depends on this prefix

60 + // eslint-disable-next-line react-internal/safe-string-coercion

61 + args = args.map(item => String(item));

62 + args.unshift('Warning: ' + format);

63 + }

64 // We intentionally don't use spread (or .apply) directly because it

65 // breaks IE9: https://github.com/facebook/react/issues/13610

66 // eslint-disable-next-line react-internal/no-production-logging

56 - Function.prototype.apply.call(console[level], console, argsWithFormat);

67 + Function.prototype.apply.call(console[level], console, args);

68 }

69 }

scripts/eslint-rules/warning-args.js

+5 -2

@@ -73,7 +73,10 @@ module.exports = {

73 );

74 return;

75 }

76 - if (format.length < 10 || /^[s\W]*$/.test(format)) {

76 + if (

77 + (format.length < 10 || /^[s\W]*$/.test(format)) &&

78 + format !== '%s\n\n%s\n'

79 + ) {

80 context.report(

81 node,

82 'The {{name}} format should be able to uniquely identify this ' +

@@ -83,7 +86,7 @@ module.exports = {

86 return;

87 }

88 // count the number of formatting substitutions, plus the first two args

86 - const expectedNArgs = (format.match(/%s/g) || []).length + 1;

89 + const expectedNArgs = (format.match(/%[so]/g) || []).length + 1;

90 if (node.arguments.length !== expectedNArgs) {

91 context.report(

92 node,