Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ReactDebugTools] wrap uncaught error from rendering user's component #24216

Merged
merged 12 commits into from
Apr 1, 2022
30 changes: 29 additions & 1 deletion packages/react-debug-tools/src/ReactDebugHooks.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
},
};
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Stylistic choice but this feels a bit cleaner to me?

let newError = error;
if (!(error instanceof Error) || error.name !== 'ReactDebugToolsUnsupportedHookError') {
  // 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
  let newError = 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
  newError.name = 'ReactDebugToolsRenderFunctionError';
}

throw newError;

}

export function inspectHooks<Props>(
renderFunction: Props => React$Node,
props: Props,
Expand All @@ -686,6 +710,8 @@ export function inspectHooks<Props>(
try {
ancestorStackError = new Error();
renderFunction(props);
} catch (error) {
handleRenderFunctionError(error);
} finally {
readHookLog = hookLog;
hookLog = [];
Expand Down Expand Up @@ -730,6 +756,8 @@ function inspectHooksOfForwardRef<Props, Ref>(
try {
ancestorStackError = new Error();
renderFunction(props, ref);
} catch (error) {
handleRenderFunctionError(error);
} finally {
readHookLog = hookLog;
hookLog = [];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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",
);
}
mondaychen marked this conversation as resolved.
Show resolved Hide resolved
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' +
Expand All @@ -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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -920,16 +920,26 @@ describe('ReactHooksInspectionIntegration', () => {

const renderer = ReactTestRenderer.create(<Foo />);
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);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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');
});

Expand Down