From c89a15c716febe71b7d857f839829cd1dc74918f Mon Sep 17 00:00:00 2001 From: "Mengdi \"Monday\" Chen" Date: Fri, 1 Apr 2022 14:38:11 -0400 Subject: [PATCH] [ReactDebugTools] wrap uncaught error from rendering user's component (#24216) * [ReactDebugTools] wrap uncaught error from rendering user's component * fix lint * make error names more package specific * update per review comments * fix tests * fix lint * fix tests * fix lint * fix error name & nits * try catch instead of mocking error * fix test for older node.js version * avoid false positive from try-catch in tests --- .../react-debug-tools/src/ReactDebugHooks.js | 30 ++++++++++++++++++- .../__tests__/ReactHooksInspection-test.js | 16 ++++++++-- .../ReactHooksInspectionIntegration-test.js | 28 +++++++++++------ .../src/__tests__/inspectedElement-test.js | 2 +- 4 files changed, 63 insertions(+), 13 deletions(-) diff --git a/packages/react-debug-tools/src/ReactDebugHooks.js b/packages/react-debug-tools/src/ReactDebugHooks.js index 49bf65e3136f..3657ed2db059 100644 --- a/packages/react-debug-tools/src/ReactDebugHooks.js +++ b/packages/react-debug-tools/src/ReactDebugHooks.js @@ -366,7 +366,7 @@ const DispatcherProxyHandler = { const error = new Error('Missing method in Dispatcher: ' + prop); // Note: This error name needs to stay in sync with react-devtools-shared // TODO: refactor this if we ever combine the devtools and debug tools packages - error.name = 'UnsupportedFeatureError'; + error.name = 'ReactDebugToolsUnsupportedHookError'; throw error; }, }; @@ -667,6 +667,30 @@ function processDebugValues( } } +function handleRenderFunctionError(error: any): void { + // original error might be any type. + if ( + error instanceof Error && + error.name === 'ReactDebugToolsUnsupportedHookError' + ) { + throw error; + } + // If the error is not caused by an unsupported feature, it means + // that the error is caused by user's code in renderFunction. + // In this case, we should wrap the original error inside a custom error + // so that devtools can give a clear message about it. + // $FlowFixMe: Flow doesn't know about 2nd argument of Error constructor + const wrapperError = new Error('Error rendering inspected component', { + cause: error, + }); + // Note: This error name needs to stay in sync with react-devtools-shared + // TODO: refactor this if we ever combine the devtools and debug tools packages + wrapperError.name = 'ReactDebugToolsRenderError'; + // this stage-4 proposal is not supported by all environments yet. + wrapperError.cause = error; + throw wrapperError; +} + export function inspectHooks( renderFunction: Props => React$Node, props: Props, @@ -686,6 +710,8 @@ export function inspectHooks( try { ancestorStackError = new Error(); renderFunction(props); + } catch (error) { + handleRenderFunctionError(error); } finally { readHookLog = hookLog; hookLog = []; @@ -730,6 +756,8 @@ function inspectHooksOfForwardRef( try { ancestorStackError = new Error(); renderFunction(props, ref); + } catch (error) { + handleRenderFunctionError(error); } finally { readHookLog = hookLog; hookLog = []; diff --git a/packages/react-debug-tools/src/__tests__/ReactHooksInspection-test.js b/packages/react-debug-tools/src/__tests__/ReactHooksInspection-test.js index 50eadbedcbe4..43624ce37aff 100644 --- a/packages/react-debug-tools/src/__tests__/ReactHooksInspection-test.js +++ b/packages/react-debug-tools/src/__tests__/ReactHooksInspection-test.js @@ -276,10 +276,20 @@ describe('ReactHooksInspection', () => { }, }; + let didCatch = false; expect(() => { - expect(() => { + // mock the Error constructor to check the internal of the error instance + try { ReactDebugTools.inspectHooks(Foo, {}, FakeDispatcherRef); - }).toThrow("Cannot read property 'useState' of null"); + } catch (error) { + expect(error.message).toBe('Error rendering inspected component'); + // error.cause is the original error + expect(error.cause).toBeInstanceOf(Error); + expect(error.cause.message).toBe( + "Cannot read property 'useState' of null", + ); + } + didCatch = true; }).toErrorDev( 'Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for' + ' one of the following reasons:\n' + @@ -289,6 +299,8 @@ describe('ReactHooksInspection', () => { 'See https://reactjs.org/link/invalid-hook-call for tips about how to debug and fix this problem.', {withoutStack: true}, ); + // avoid false positive if no error was thrown at all + expect(didCatch).toBe(true); expect(getterCalls).toBe(1); expect(setterCalls).toHaveLength(2); diff --git a/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js b/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js index f5003e96bdf6..e57f084975d6 100644 --- a/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js +++ b/packages/react-debug-tools/src/__tests__/ReactHooksInspectionIntegration-test.js @@ -920,16 +920,26 @@ describe('ReactHooksInspectionIntegration', () => { const renderer = ReactTestRenderer.create(); const childFiber = renderer.root._currentFiber(); - expect(() => { + + let didCatch = false; + + try { ReactDebugTools.inspectHooksOfFiber(childFiber, FakeDispatcherRef); - }).toThrow( - 'Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for' + - ' one of the following reasons:\n' + - '1. You might have mismatching versions of React and the renderer (such as React DOM)\n' + - '2. You might be breaking the Rules of Hooks\n' + - '3. You might have more than one copy of React in the same app\n' + - 'See https://reactjs.org/link/invalid-hook-call for tips about how to debug and fix this problem.', - ); + } catch (error) { + expect(error.message).toBe('Error rendering inspected component'); + expect(error.cause).toBeInstanceOf(Error); + expect(error.cause.message).toBe( + 'Invalid hook call. Hooks can only be called inside of the body of a function component. This could happen for' + + ' one of the following reasons:\n' + + '1. You might have mismatching versions of React and the renderer (such as React DOM)\n' + + '2. You might be breaking the Rules of Hooks\n' + + '3. You might have more than one copy of React in the same app\n' + + 'See https://reactjs.org/link/invalid-hook-call for tips about how to debug and fix this problem.', + ); + didCatch = true; + } + // avoid false positive if no error was thrown at all + expect(didCatch).toBe(true); expect(getterCalls).toBe(1); expect(setterCalls).toHaveLength(2); diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index 919fbb059f76..81c0a3a75821 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -2145,7 +2145,7 @@ describe('InspectedElement', () => { expect(value).toBe(null); const error = errorBoundaryInstance.state.error; - expect(error.message).toBe('Expected'); + expect(error.message).toBe('Error rendering inspected component'); expect(error.stack).toContain('inspectHooksOfFiber'); });