feat: use error boundary to capture useEffect errors #539
Conversation
Codecov Report
@@ Coverage Diff @@
## beta #539 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 14 14
Lines 202 210 +8
Branches 27 23 -4
=========================================
+ Hits 202 210 +8
Continue to review full report at Codecov.
|
| export async function callAfter(callback: () => void, ms: number) { | ||
| async function callAfter(callback: () => void, ms: number) { |
There was a problem hiding this comment.
moved the the export at the end of the file
| try { | ||
| const serverOutput = ReactDOMServer.renderToString(testHarness(props)) | ||
| container.innerHTML = serverOutput | ||
| } catch (e: unknown) { | ||
| rendererProps.setError(e as Error) | ||
| } |
There was a problem hiding this comment.
Turns out error boundaries throw synchronously when server rendering. This is the only renderer that has any special treatment for error handling and only in the initial render function. The error boundary behaves normally once it has been hydrated.
| const TestComponent = ({ hookProps }: { hookProps?: TProps }) => { | ||
| // coerce undefined into TProps, so it maintains the previous behaviour | ||
| setValue(callback(hookProps as TProps)) | ||
| return null | ||
| } |
There was a problem hiding this comment.
I brought this into createTestHarness because:
ErrorFallbackneeded to be inside and I like consistency- It simplified the
propsofTestComponentas therendererPropsare already available in the outer scope
There was a problem hiding this comment.
I think I originally moved it out before we moved out testHarness so It makes sense to put it back in since TestComponent isn't required on it's own.
|
🎉 This PR is included in version 5.0.0-beta.12 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 5.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What:
Replace
try/catcherror handling with a an error boundary.Why:
Fixes #308
The error boundary will catch errors thrown in
useEffectcalls as well as in the render path.How:
The challenge here and why we haven't done it sooner is the excessive amount of logging produced from React when an error boundary catches and error. I've tried many official and unofficial approaches over the years but none have worked consistently as we changed renderers and new versions of React were released. For more information, see these issues:
In this proposal, I used a package called filter-console to prevent errors matching a specific patterns from being logged. This is potentially brittle if new versions of React change the log format, but we can add more patterns or change the existing ones if they ever do 🤷♂️
I also opted to use
react-error-boundaryas it provided some nice features for resetting the error state when rerendering. The way I access the reset functionality is a bit clunky, so I'm open to suggestions on how to clean that up.Checklist:
We had a bunch of tests marked as
.skipfor testing the effect errors that I have unmarked and they all work now, which is a good sign.The other tests that needed updating was the
result history tests. The hook used in those tests actually had a side effect from rendering that thetry/catchwas hiding, but the error boundary showed the results appearing out of order because the error handling triggered an additional render that cause the next result to jump ahead in the array. It was painful to debug, but the new tests worked against the old implementation and as well as the new one.