-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
fix(core): log missing errorInfo in React 18 onRecoverableError callback #9387
Conversation
✅ [V2]
To edit notification comments on pull requests, go to your Netlify site configuration. |
⚡️ Lighthouse report for the deploy preview of this PR
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like lint/prettier fails
@@ -37,8 +37,8 @@ if (ExecutionEnvironment.canUseDOM) { | |||
</HelmetProvider> | |||
); | |||
|
|||
const onRecoverableError = (error: unknown): void => { | |||
console.error('Docusaurus React Root onRecoverableError:', error); | |||
const onRecoverableError = (error: unknown, errorInfo: { digest?: string | null, componentStack?: string | null }): void => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, missed that we could get some additional info in the callback
Why not using the official type ReactDOM.ErrorInfo?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the official type ReactDOM.ErrorInfo
Oh. where is that type found? I used the implementation I found in the React codebase - do you have another link?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't show up in the repo? https://github.com/search?q=repo%3Afacebook%2Freact%20ReactDOM.ErrorInfo&type=code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, will try to fix it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably only exists on external typedefs, fixed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh so the type you're using is from DefinitelyTyped: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/29f3f65685aba9e9b8a105a55494923980e2f16e/types/react/index.d.ts#L3240
Ironically, I'm one of the maintainers of that. It's slightly inaccurate too :-). But it doesn't really matter - we just want the console.log
to take place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've raised a PR to update the Definitely Typed definition to be more accurate: DefinitelyTyped/DefinitelyTyped#66987
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes I know that's why I didn't understand 😅 great if you catched a typo
Pre-flight checklist
Motivation
See #9379
Test Plan
I hope to use this to diagose #9379
Test links
Related issues/PRs
See #9379