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

ENH add react boundary to catch rendering errors #24

Merged
merged 5 commits into from
Apr 19, 2024

Conversation

Charlie-XIAO
Copy link
Contributor

@Charlie-XIAO Charlie-XIAO commented Apr 12, 2024

Previously, errors within React functional components cannot be caught properly, e.g. this widget. They will thus not be displayed on the canvas which is undesired.

This PR wraps each user widget component with an error boundary using react-error-boundary, which is recommended in the docs. The rendering would look like this:

image

Note, however, that the error will still exist in the console (xref facebook/react#15069). May not matter that much for end users, but would be somehow annoying for debugging if it keeps spamming up the console.

Some other arguable points of this PR are:

  • Is grabErrorInfo put in the right place? Is it correct to have two utils module organized like this?
  • Is the title "Rendering error caught by the React error boundary" proper? And perhaps we want to reconsider other error titles at some point when the UI of the canvas is improved.

@Charlie-XIAO Charlie-XIAO added the type:enhancement New feature or request label Apr 12, 2024
Copy link
Contributor

@ROMEEZHOU ROMEEZHOU left a comment

Choose a reason for hiding this comment

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

For the naming of the title, perhaps 1. also show the id of the widget; 2. use a more intuitive expression than "error boundary" (cause one may not know what error boundary specifically means?)

@Charlie-XIAO
Copy link
Contributor Author

Charlie-XIAO commented Apr 16, 2024

For the naming of the title, perhaps 1. also show the id of the widget; 2. use a more intuitive expression than "error boundary" (cause one may not know what error boundary specifically means?)

Valid points. I've updated all existing error messages to be consistent, reflect the widget ID, and be as user-friendly and accurate as possible. (We may update them again in the future when the framework gets updated or when we finds out corner cases via frontend tests.)

Copy link
Contributor

@ROMEEZHOU ROMEEZHOU left a comment

Choose a reason for hiding this comment

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

LGTM

@ROMEEZHOU ROMEEZHOU merged commit 6fef9e8 into main Apr 19, 2024
6 checks passed
@Charlie-XIAO Charlie-XIAO deleted the react-error-boundary branch June 18, 2024 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants