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

feat(POC): add error boundary to withSuspense #10468

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gkartalis
Copy link
Member

@gkartalis gkartalis commented Jul 10, 2024

This PR resolves PHIRE-1019 and PHIRE-1109

Description

Introduced the sentry error boundary component that we can use instead of the standard react one.

Benefits over the react one are that the errors go to sentry directly and if we want we can also add a sentry feedback button (if we want) for the user to describe what they encountered without too much lift its pretty much ready from their side.

The Sentry Error boundary is basically the same - provides an error boundary component with a fallback prop (it can be any component we want similar to what we do have in react)

More information for the errorBoundary can be found here

More information for the user feedback flow can be found here

For now the solution is not ideal (that's why I marked it as #DONOTMERGE) since it will display in the existing places that we do have the withSuspense function the spinner in case of error.

Happy to gather some feedback or ideas to understand how to proceed.

How to test

You can connect local metaphysics to eigen locally and throw an error from the query from any point that we do use the withSuspense

PR Checklist

  • I have tested my changes on iOS and Android.
  • I hid my changes behind a feature flag, or they don't need one.
  • I have included screenshots or videos, or I have not changed the UI.
  • I have added tests, or my changes don't require any.
  • I added an app state migration, or my changes do not require one.
  • I have documented any follow-up work that this PR will require, or it does not require any.
  • I have added a changelog entry below, or my changes do not require one.

To the reviewers 👀

  • I would like at least one of the reviewers to run this PR on the simulator or device.
Changelog updates

Changelog updates

Cross-platform user-facing changes

iOS user-facing changes

Android user-facing changes

Dev changes

  • add error boundary to withSuspense - gkartalis

Need help with something? Have a look at our docs, or get in touch with us.

@gkartalis gkartalis self-assigned this Jul 10, 2024
@ArtsyOpenSource
Copy link
Contributor

ArtsyOpenSource commented Jul 10, 2024

This PR contains the following changes:

  • Dev changes (add error boundary to withSuspense - gkartalis)

Generated by 🚫 dangerJS against 9f60572

brainbicycle
brainbicycle previously approved these changes Aug 14, 2024
Copy link
Contributor

@brainbicycle brainbicycle left a comment

Choose a reason for hiding this comment

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

this looks good to me. I would actually be in favor of testing and merging because I believe as it stands today these network errors are essentially unhandled and lead to render error that then bubble up to our app wide error boundary which essentially blanks out the whole screen with the unable to load screen. In an ideal world we have thoughtful error states everywhere but I still think this is better than where we are at now.

As far as testing I wonder if there is somewhere we can just arbitrarily throw an error, maybe in relay code when using the withSuspense hook, rather than messing with connecting local metaphysics and throwing for specific queries.

@brainbicycle brainbicycle changed the title feat(POC): add error boundary to withSuspense #DONOTMERGE feat(POC): add error boundary to withSuspense Sep 12, 2024
@brainbicycle
Copy link
Contributor

This has been discussed a bit more and I think we should merge this, it is an improvement on what happens today.

@brainbicycle brainbicycle added the Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green label Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Jira Synced Squash On Green A label to indicate that Peril should squash-merge this PR when all statuses are green
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants