-
Notifications
You must be signed in to change notification settings - Fork 58
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
CPLAT-5599 Add temporary JS error boundary #291
Conversation
Security InsightsNo security relevant content was detected by automated scans. Action Items
Questions or Comments? Reach out on Slack: #support-infosec. |
b897997
to
15e129a
Compare
15e129a
to
32a5cb0
Compare
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.
+1
+ Do not show in IDE analyzer view
Codecov Report
@@ Coverage Diff @@
## 3.0.0-wip #291 +/- ##
=============================================
+ Coverage 90.27% 90.45% +0.19%
=============================================
Files 36 36
Lines 1807 1821 +14
=============================================
+ Hits 1631 1647 +16
+ Misses 176 174 -2 |
@greglittlefield-wf feedback addressed |
+10 Will merge once travis passes |
@Workiva/release-management-pp |
Motivation
Problem
We want to release react-dart
5.0.0
(which upgrades us to ReactJS 16) without depending on Dart 2 /Component2
/UiComponent2
- and all the new lifecycle methods that those expose.However, one of the biggest behavioral differences between ReactJS 15 and 16 is that in 16, when a component throws - the entire component tree that it is a part of - will unmount completely.
This means that if we just unleash ReactJS 16 within the Workiva ecosystem, if any UI component in the entire shell throws - the entire shell will unmount (blank screen). This is an unacceptable risk / experience.
Solution
In CPLAT-3093, we created an
ErrorBoundary
component in over_react - and have since wrapped all critical "top level"react_dom.render
calls within Workiva platform repos to make thatErrorBoundary
component be the root of the tree.We can make use of that to mitigate our problem by having it actually wrap around a new, private JS component that utilizes its own JS interop to make use of the ReactJS
componentDidCatch
/getDerivedStateFromError
lifecycle methods to prevent the entire tree from unmounting when an error is caught.Changes
componentDidCatch
/getDerivedStateFromError
lifecycle methods.ErrorBoundary
component to render the new private JS component, passingprops.children
through as the children of the private JS component.props.onComponentDidCatch
callback using the JS component'scomponentDidCatch
lifecycle method interop handler.Review
See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.
Please review: @greglittlefield-wf @joebingham-wk @kealjones-wk
QA Checklist
ErrorBoundary
, verify that the button is still rendered in the DOM after the error is thrown.Merge Checklist
While we perform many automated checks before auto-merging, some manual checks are needed: