feat: Error boundary component #3736
Conversation
pan-kot
force-pushed
the
error-boundaries
branch
2 times, most recently
from
2826547 to
84097f7
Compare
pan-kot
force-pushed
the
error-boundaries
branch
3 times, most recently
from
b52bd30 to
df1bd58
Compare
pan-kot
force-pushed
the
error-boundaries
branch
from
df1bd58 to
4f96ccd
Compare
pan-kot
force-pushed
the
error-boundaries
branch
3 times, most recently
from
1f1f616 to
762147a
Compare
pan-kot
force-pushed
the
error-boundaries
branch
from
762147a to
720060d
Compare
pan-kot
force-pushed
the
error-boundaries
branch
from
720060d to
4694f5b
Compare
pan-kot
force-pushed
the
error-boundaries
branch
8 times, most recently
from
9c9b0ef to
bc8b8de
Compare
pan-kot
force-pushed
the
error-boundaries
branch
2 times, most recently
from
6e677ac to
1b6cea2
Compare
| </div> | ||
| {activeDrawerId !== TOOLS_DRAWER_ID && ( | ||
| <div className={styles['drawer-content']} style={{ blockSize: drawerHeight }}> | ||
| <div key={activeDrawerId} className={styles['drawer-content']} style={{ blockSize: drawerHeight }}> |
There was a problem hiding this comment.
Passing React key forces component re-mount when new drawer is rendered. This clears the Drawer's internal state including the error state of the built-in error boundary. Without this, if an error boundary is triggered in one drawer - it will be shown in all drawers.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3736 +/- ##
==========================================
- Coverage 97.13% 97.12% -0.01%
==========================================
Files 866 870 +4
Lines 25399 25488 +89
Branches 9183 9215 +32
==========================================
+ Hits 24671 24756 +85
+ Misses 722 685 -37
- Partials 6 47 +41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pan-kot
force-pushed
the
error-boundaries
branch
from
1b6cea2 to
0e51999
Compare
| [styles['header-with-media']]: hasMedia, | ||
| [styles['header-full-page']]: __fullPage && isRefresh, | ||
| } | ||
| <BuiltInErrorBoundary wrapper={content => <InternalBox padding="m">{content}</InternalBox>}> |
There was a problem hiding this comment.
The container paddings are implemented separately in its header, body, and footer. When we replace all these slots with the error boundary fallback, these paddings are lost - that's why we wrap the fallback in a padded box.
| > | ||
| <BuiltInErrorBoundary | ||
| wrapper={content => ( | ||
| <InternalBox padding={disableContentPaddings ? 'm' : undefined}>{content}</InternalBox> |
There was a problem hiding this comment.
When disableContentPaddings is used, the container has no internal paddings around content, expecting the paddings to come from the content. However, the internal content paddings are lost in case there is an error in the content - that's why we add them back with a padded box around the fallback message.
There was a problem hiding this comment.
Thank you for these nice explanations. Why not incorporate them as code comments (maybe in a slightly more succinct form)?
There was a problem hiding this comment.
Good point - I will add the code comments 👍
src/error-boundary/internal.tsx
Outdated
| } | ||
|
|
||
| componentDidCatch(error: Error, errorInfo: React.ErrorInfo): void { | ||
| this.props.onError({ error, errorInfo, errorBoundaryId: this.props.errorBoundaryId }); |
There was a problem hiding this comment.
This change breaks the unit tests below, which assert we are permissive and don't crash if not providing the onError prop (even if it is required)
pan-kot
force-pushed
the
error-boundaries
branch
from
1f61670 to
c185f12
Compare
pan-kot
force-pushed
the
error-boundaries
branch
from
023c879 to
7b6a0f7
Compare
This reverts commit 94344c6.
2 participants
Description
See: [3akTAfFaeBUk]
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.