Skip to content

fix(react): Add children prop type to ErrorBoundary component #4966

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Apr 26, 2022
Merged

fix(react): Add children prop type to ErrorBoundary component #4966

Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a575e6
Add children prop to ErrorBoundary prop type alias
cameronaziz Apr 23, 2022
ea99673
Add Support for Function Child Component
cameronaziz Apr 25, 2022
ae050de
use PropsWithChildren
cameronaziz Apr 25, 2022
d9231c3
Add children prop to ErrorBoundry
lforst Apr 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/react/src/errorboundary.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ const INITIAL_STATE = {
* Sentry React SDK ErrorBoundary caught an error invoking your application code. This
* is expected behavior and NOT indicative of a bug with the Sentry React SDK.
*/
class ErrorBoundary extends React.Component<ErrorBoundaryProps, ErrorBoundaryState> {
class ErrorBoundary extends React.Component<React.PropsWithChildren<ErrorBoundaryProps>, ErrorBoundaryState> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @cameronaziz, thank you so much for submitting this PR. We would like to fix this type error and release a patch.

Would you be willing to get rid of the React.PropsWithChildren helper here and instead add a children?: React.ReactNode prop to ErrorBoundaryProps, just like one of the maintainers of react suggests here? If that's done we're good to go!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lforst Are you thinking more on the direction of what my original commit was: ea99673

The issue was it would not build with the children render as a function.

Copy link
Contributor

Choose a reason for hiding this comment

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

1a575e6 without the | undefined would be perfect imo.

That should be fine typewise. Seems weird that it's not building. Can you update the code? I'll take a look at the build output afterwards.

Copy link
Contributor Author

@cameronaziz cameronaziz Apr 25, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I can, but that ends up making the children within this block a never.

sentry-javascript/packages/react/src/errorboundary.tsx

Lines 153 to 155 in cafec74

if (typeof children === 'function') {
return children();
}

Copy link
Contributor

Choose a reason for hiding this comment

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

You're right. Apparently @types/react@18 changed the ReactNode type not to be callable anymore. I think up to v17 it had a constructor type of some sort. For me, when using the types from @type/react@18 neither children?: ReactNode nor React.PropsWithChildren works. I think it's best if we go with the changes from ea99673.

Sorry for flip-flopping around. Thank you very much for helping out 🙏

public state: ErrorBoundaryState = INITIAL_STATE;

public componentDidCatch(error: Error & { cause?: Error }, { componentStack }: React.ErrorInfo): void {
Expand Down