-
Notifications
You must be signed in to change notification settings - Fork 573
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 #10824
Conversation
Important Before merging we need to test out how the default error fallback will behave in the already existing surfaces. Ideas welcome on that @damassi @MounirDhahri @brainbicycle |
How does the new |
in this one we basically can override the behaviour with any fallback. But the default is null so it creates a dead end if it is used by a screen. On the other hand if it is a rail it just defensively hides it |
By dead end do you mean we will show this? |
src/app/Scenes/NewWorksFromGalleriesYouFollow/Components/NewWorksFromGalleriesYouFollow.tsx
Outdated
Show resolved
Hide resolved
this ready for review! @gkartalis |
> | ||
<Component {...props} /> | ||
</Suspense> | ||
</ErrorBoundary> |
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.
main change, make error fallback required prop and use the react-error-boundary
+ | ||
+ | ||
+ const metaphysicsRejectionTests = { | ||
+ query: "query HomeAboveTheFoldQuery", |
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.
this is a test patch that can be used for testing failures, move to patches, reinstall deps, then update the query here to cause it to reject
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.
forgot a step, need to turn off persisted queries to effectively match queries, I may add a dev toggle for that if we don't remove it entirely
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.
amazing, looks great! Left only a small question. I can't approve since I am the one who opened the PR but consider it approved
src/app/Scenes/HomeView/Sections/HomeViewSectionFeaturedCollection.tsx
Outdated
Show resolved
Hide resolved
For the Amazing work y'all! This is a really huge improvement. |
good point the loadfailureview does support that but we need to pass a function to onRetry, let me see if anything makes sense here. |
done! |
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.
+1000
…rrent Galleries section component (#10922) * feat: support HomeViewSectionCard with a default full-width presentation * chore: remove HomeViewSectionGalleries This section type was a bit of a misnomer and never actually served up a galleries connection. We remove it here, with the possibility of bring it back such a component when it is actually needed * refactor: allow navigating to route that is a non-canonical web href * fix: withSuspense usage after #10824 * chore: bump schema
This PR resolves PHIRE-1019 and PHIRE-1109
Description
Introduced an errorFallback component in case something goes wrong, devs can override by passing their custom
errorfallback
.We also introduced
react-error-boundary
dep that is recommended from react official docs over the standard react one and also utilized theonError
callback to report the errors on Sentry.Makes
withSuspense
hook now require passing a fallback component.Stops tracking updated components that are now not dead ends.
Updates all components now using
withSuspense
to pass a fallback using following patterns:Subcomponents whose absences does not hinder the user much just don't render it in case of error - examples - home screen sections and filter sections:
Examples
Full screen component and there is an existing error component available show that, make sure nav is available
Examples
Full screen component with no error component available, use our LoadFailureView, make sure nav is available
Examples
Follow-ups
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
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.