-
Notifications
You must be signed in to change notification settings - Fork 6
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: add error boundaries and suspense usage with TRPC #93
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
// The choice to not redirect via next's router was intentional to handle ErrorBoundary for the app root | ||
// Using next's router.push('/sign-in') will not render the SignIn component as it won't be mounted in the app root as the ErrorBoundary fallback component will be rendered instead | ||
// Using vanilla location redirecting will prompt a full page reload of /sign-in page, which will never trigger the root ErrorBoundary, thus rendering the full component correctly | ||
if (res.data === 'UNAUTHORIZED') { | ||
window.location.href = 'sign-in' | ||
return | ||
} |
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.
e2e tests failing, but fixed in #94. Can rebase/merge develop in first? |
Co-authored-by: Ian @pregnantboy
af6d876
to
03d48e6
Compare
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.
lgtm
Problem
Problem 1
Our current method of fetching data for pages with query params utilize
getServerSideProps
. We noticed a trend of slow initial load times for these pages as long as ~7s. This is likely (but not confirmed) attributed to Chakra's huge bundle size which slows down the time needed to build the server rendered page.Solutions considered to root problem
We had two options
getServerSideProps
's bundle issues with Chakra - possibly looking into how we can treeshake chakra for server rendered pages to reduce build timesWe went with option 2 as that was already in the existing roadmap.
Problem 2
Existing CSR patterns with page based routing was cumbersome as we could not use React 18's
Suspense
and the defaultErrorBoundary
fromreact-error-boundary
.This led to 2 dx issues:
isLoading
andisError
states and render the fallback components accordinglyErrorBoundary
which would catch any errors thrown within child components whichuseSuspenseQuery
Solution
Suspense
component was due to the fact that any child component which calls theuseRouter
hook erroring out during server side rendering as the router had not mounted. TSuspense
wrapper that keeps track of aisMounted
state.Suspense
with the childrenfallbackComponent
will render.ErrorBoundary
wrapper which catches error boundary states to render the correct fallback componentsImprovements:
useSuspenseQuery
sign-in
anddashboard
sign-in
. This logic for this is now located in theErrorBoundary
component whenever a TRPCError has a code ofUNAUTHORIZED
Tests
dashboard
without logging in succesfully redirects me tosign-in
sign-in
whilst authed redirects me to eithercallbackUrl
ordashboard
ErrorBoundary
fallback component renders on different TRPCError codesSuspense
fallback renders when data is suspending