From 4de28964086b0bab2e7ffab8951d433d24c04e6b Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 10 Jun 2021 23:00:08 +0300 Subject: [PATCH 1/8] feat: Crate ErrorBoundary component --- src/components/ErrorBoundary.js | 52 +++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 src/components/ErrorBoundary.js diff --git a/src/components/ErrorBoundary.js b/src/components/ErrorBoundary.js new file mode 100644 index 000000000000..fb246425f8da --- /dev/null +++ b/src/components/ErrorBoundary.js @@ -0,0 +1,52 @@ +import React from 'react'; +import PropTypes from 'prop-types'; +import crashlytics from '@react-native-firebase/crashlytics'; + +import Log from '../libs/Log'; + +const propTypes = { + /* An message posted to the server (along with the error) when this component intercepts an error */ + errorMessage: PropTypes.string.isRequired, + + /* Actual content wrapped by this error boundary */ + children: PropTypes.node.isRequired, +}; + +/** + * This component captures an error in the child component tree and logs it to the server + * It can be used to wrap the entire app as well as to wrap specific parts for more granularity + * @see {@link https://reactjs.org/docs/error-boundaries.html#where-to-place-error-boundaries} + */ +class ErrorBoundary extends React.Component { + constructor(props) { + super(props); + this.state = {hasError: false}; + } + + static getDerivedStateFromError() { + // Update state so the next render will show the fallback UI. + return {hasError: true}; + } + + componentDidCatch(error, errorInfo) { + // Log the error to the server + Log.info(this.props.errorMessage, true, {error, errorInfo}); + + // Since the error was handled we need to manually tell crashlytics about it + crashlytics().log(`errorInfo: ${errorInfo}`); + crashlytics().recordError(error); + } + + render() { + if (this.state.hasError) { + // For the moment we've decided not to render any fallback UI + return null; + } + + return this.props.children; + } +} + +ErrorBoundary.propTypes = propTypes; + +export default ErrorBoundary; From 52102f0a958552a35d09084d0ba5f9dd01b25fa7 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Thu, 10 Jun 2021 23:21:09 +0300 Subject: [PATCH 2/8] feat: wrap the root component with an error boundary --- src/App.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/App.js b/src/App.js index 58845d56e429..e1635c973422 100644 --- a/src/App.js +++ b/src/App.js @@ -1,12 +1,16 @@ import React from 'react'; import {SafeAreaProvider} from 'react-native-safe-area-context'; + import CustomStatusBar from './components/CustomStatusBar'; import Expensify from './Expensify'; +import ErrorBoundary from './components/ErrorBoundary'; const App = () => ( - + + + ); From 5100744df2e6a7ad6b1435638323858b8fdeaa9c Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 11 Jun 2021 00:14:39 +0300 Subject: [PATCH 3/8] refactor: extract platform specific implementation due to crashlytics logging @react-native-firebase/crashlytics does not work for web and desktop --- .../BaseErrorBoundary.js} | 26 +++++++++---------- src/components/ErrorBoundary/index.js | 9 +++++++ src/components/ErrorBoundary/index.native.js | 16 ++++++++++++ 3 files changed, 38 insertions(+), 13 deletions(-) rename src/components/{ErrorBoundary.js => ErrorBoundary/BaseErrorBoundary.js} (65%) create mode 100644 src/components/ErrorBoundary/index.js create mode 100644 src/components/ErrorBoundary/index.native.js diff --git a/src/components/ErrorBoundary.js b/src/components/ErrorBoundary/BaseErrorBoundary.js similarity index 65% rename from src/components/ErrorBoundary.js rename to src/components/ErrorBoundary/BaseErrorBoundary.js index fb246425f8da..aee0f94a5665 100644 --- a/src/components/ErrorBoundary.js +++ b/src/components/ErrorBoundary/BaseErrorBoundary.js @@ -1,23 +1,27 @@ import React from 'react'; import PropTypes from 'prop-types'; -import crashlytics from '@react-native-firebase/crashlytics'; - -import Log from '../libs/Log'; const propTypes = { - /* An message posted to the server (along with the error) when this component intercepts an error */ + /* A message posted to `logError` (along with error data) when this component intercepts an error */ errorMessage: PropTypes.string.isRequired, + /* A function to perform the actual logging since different platforms support different tools */ + logError: PropTypes.func, + /* Actual content wrapped by this error boundary */ children: PropTypes.node.isRequired, }; +const defaultProps = { + logError: () => {}, +}; + /** * This component captures an error in the child component tree and logs it to the server * It can be used to wrap the entire app as well as to wrap specific parts for more granularity * @see {@link https://reactjs.org/docs/error-boundaries.html#where-to-place-error-boundaries} */ -class ErrorBoundary extends React.Component { +class BaseErrorBoundary extends React.Component { constructor(props) { super(props); this.state = {hasError: false}; @@ -29,12 +33,7 @@ class ErrorBoundary extends React.Component { } componentDidCatch(error, errorInfo) { - // Log the error to the server - Log.info(this.props.errorMessage, true, {error, errorInfo}); - - // Since the error was handled we need to manually tell crashlytics about it - crashlytics().log(`errorInfo: ${errorInfo}`); - crashlytics().recordError(error); + this.props.logError(this.props.errorMessage, error, errorInfo); } render() { @@ -47,6 +46,7 @@ class ErrorBoundary extends React.Component { } } -ErrorBoundary.propTypes = propTypes; +BaseErrorBoundary.propTypes = propTypes; +BaseErrorBoundary.defaultProps = defaultProps; -export default ErrorBoundary; +export default BaseErrorBoundary; diff --git a/src/components/ErrorBoundary/index.js b/src/components/ErrorBoundary/index.js new file mode 100644 index 000000000000..345b0b902357 --- /dev/null +++ b/src/components/ErrorBoundary/index.js @@ -0,0 +1,9 @@ +import BaseErrorBoundary from './BaseErrorBoundary'; +import Log from '../../libs/Log'; + +BaseErrorBoundary.defaultProps.logError = (errorMessage, error, errorInfo) => { + // Log the error to the server + Log.info(errorMessage, true, {error, errorInfo}); +}; + +export default BaseErrorBoundary; diff --git a/src/components/ErrorBoundary/index.native.js b/src/components/ErrorBoundary/index.native.js new file mode 100644 index 000000000000..66e031bac4ed --- /dev/null +++ b/src/components/ErrorBoundary/index.native.js @@ -0,0 +1,16 @@ +import crashlytics from '@react-native-firebase/crashlytics'; + +import BaseErrorBoundary from './BaseErrorBoundary'; +import Log from '../../libs/Log'; + +BaseErrorBoundary.defaultProps.logError = (errorMessage, error, errorInfo) => { + // Log the error to the server + Log.info(this.props.errorMessage, true, {error, errorInfo}); + + /* On native we also log the error to crashlytics + * Since the error was handled we need to manually tell crashlytics about it */ + crashlytics().log(`errorInfo: ${errorInfo}`); + crashlytics().recordError(error); +}; + +export default BaseErrorBoundary; From 48eb85d3166dde2b4359455244f6e8847684e899 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 11 Jun 2021 00:35:46 +0300 Subject: [PATCH 4/8] refactor: use Log.alert instead of Log.info --- src/components/ErrorBoundary/index.js | 2 +- src/components/ErrorBoundary/index.native.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/ErrorBoundary/index.js b/src/components/ErrorBoundary/index.js index 345b0b902357..a13bcd17bed3 100644 --- a/src/components/ErrorBoundary/index.js +++ b/src/components/ErrorBoundary/index.js @@ -3,7 +3,7 @@ import Log from '../../libs/Log'; BaseErrorBoundary.defaultProps.logError = (errorMessage, error, errorInfo) => { // Log the error to the server - Log.info(errorMessage, true, {error, errorInfo}); + Log.alert(errorMessage, 10, {error, errorInfo}); }; export default BaseErrorBoundary; diff --git a/src/components/ErrorBoundary/index.native.js b/src/components/ErrorBoundary/index.native.js index 66e031bac4ed..3dddfd49135c 100644 --- a/src/components/ErrorBoundary/index.native.js +++ b/src/components/ErrorBoundary/index.native.js @@ -5,7 +5,7 @@ import Log from '../../libs/Log'; BaseErrorBoundary.defaultProps.logError = (errorMessage, error, errorInfo) => { // Log the error to the server - Log.info(this.props.errorMessage, true, {error, errorInfo}); + Log.alert(errorMessage, 10, {error, errorInfo}); /* On native we also log the error to crashlytics * Since the error was handled we need to manually tell crashlytics about it */ From c021123388c73d64603ae7f3cecc95237c62e986 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 11 Jun 2021 00:42:12 +0300 Subject: [PATCH 5/8] test: fix failing "loginTest" --- tests/unit/loginTest.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/tests/unit/loginTest.js b/tests/unit/loginTest.js index c8bcc6c6f00f..02425b6dfa68 100644 --- a/tests/unit/loginTest.js +++ b/tests/unit/loginTest.js @@ -9,6 +9,11 @@ import React from 'react'; import renderer from 'react-test-renderer'; import App from '../../src/App'; +jest.mock('@react-native-firebase/crashlytics', () => ({ + log: jest.fn(), + recordError: jest.fn(), +})); + describe('AppComponent', () => { it('renders correctly', () => { renderer.create(); From 80d530979153ae6968ebc2825a579c16b5af3552 Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 11 Jun 2021 01:16:09 +0300 Subject: [PATCH 6/8] refactor: update parameters because the error message is not logging The error logs as "{}" in the log, probably because it's a special object Updated to pass the error message instead --- src/components/ErrorBoundary/index.js | 2 +- src/components/ErrorBoundary/index.native.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/ErrorBoundary/index.js b/src/components/ErrorBoundary/index.js index a13bcd17bed3..7c4a5b2ac23a 100644 --- a/src/components/ErrorBoundary/index.js +++ b/src/components/ErrorBoundary/index.js @@ -3,7 +3,7 @@ import Log from '../../libs/Log'; BaseErrorBoundary.defaultProps.logError = (errorMessage, error, errorInfo) => { // Log the error to the server - Log.alert(errorMessage, 10, {error, errorInfo}); + Log.alert(errorMessage, 10, {error: error.message, errorInfo}); }; export default BaseErrorBoundary; diff --git a/src/components/ErrorBoundary/index.native.js b/src/components/ErrorBoundary/index.native.js index 3dddfd49135c..2a5e44da0168 100644 --- a/src/components/ErrorBoundary/index.native.js +++ b/src/components/ErrorBoundary/index.native.js @@ -5,7 +5,7 @@ import Log from '../../libs/Log'; BaseErrorBoundary.defaultProps.logError = (errorMessage, error, errorInfo) => { // Log the error to the server - Log.alert(errorMessage, 10, {error, errorInfo}); + Log.alert(errorMessage, 10, {error: error.message, errorInfo}); /* On native we also log the error to crashlytics * Since the error was handled we need to manually tell crashlytics about it */ From 570845a89adfd0153dd4f044a050e8d510b02a3c Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 11 Jun 2021 01:22:35 +0300 Subject: [PATCH 7/8] refactor: remove stack trace and set captured messages The stack trace is not interesting as it just traces to the moment we call `Log.alert` in `logError` Also the `errorInfo` contains the actual stack from React --- src/components/ErrorBoundary/index.js | 2 +- src/components/ErrorBoundary/index.native.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/ErrorBoundary/index.js b/src/components/ErrorBoundary/index.js index 7c4a5b2ac23a..7e8fdfdc2131 100644 --- a/src/components/ErrorBoundary/index.js +++ b/src/components/ErrorBoundary/index.js @@ -3,7 +3,7 @@ import Log from '../../libs/Log'; BaseErrorBoundary.defaultProps.logError = (errorMessage, error, errorInfo) => { // Log the error to the server - Log.alert(errorMessage, 10, {error: error.message, errorInfo}); + Log.alert(errorMessage, 0, {error: error.message, errorInfo}, false); }; export default BaseErrorBoundary; diff --git a/src/components/ErrorBoundary/index.native.js b/src/components/ErrorBoundary/index.native.js index 2a5e44da0168..ad18d5046b49 100644 --- a/src/components/ErrorBoundary/index.native.js +++ b/src/components/ErrorBoundary/index.native.js @@ -5,7 +5,7 @@ import Log from '../../libs/Log'; BaseErrorBoundary.defaultProps.logError = (errorMessage, error, errorInfo) => { // Log the error to the server - Log.alert(errorMessage, 10, {error: error.message, errorInfo}); + Log.alert(errorMessage, 0, {error: error.message, errorInfo}, false); /* On native we also log the error to crashlytics * Since the error was handled we need to manually tell crashlytics about it */ From d01196e0766e158530b1aabcb330bde1c37cd7af Mon Sep 17 00:00:00 2001 From: Peter Velkov Date: Fri, 11 Jun 2021 11:54:56 +0300 Subject: [PATCH 8/8] refactor: address PR comments --- src/App.js | 5 ++--- src/components/ErrorBoundary/index.native.js | 2 +- tests/unit/loginTest.js | 4 +++- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/App.js b/src/App.js index e1635c973422..c69e6da22cb8 100644 --- a/src/App.js +++ b/src/App.js @@ -1,14 +1,13 @@ import React from 'react'; import {SafeAreaProvider} from 'react-native-safe-area-context'; - import CustomStatusBar from './components/CustomStatusBar'; -import Expensify from './Expensify'; import ErrorBoundary from './components/ErrorBoundary'; +import Expensify from './Expensify'; const App = () => ( - + diff --git a/src/components/ErrorBoundary/index.native.js b/src/components/ErrorBoundary/index.native.js index ad18d5046b49..d320b46984e0 100644 --- a/src/components/ErrorBoundary/index.native.js +++ b/src/components/ErrorBoundary/index.native.js @@ -9,7 +9,7 @@ BaseErrorBoundary.defaultProps.logError = (errorMessage, error, errorInfo) => { /* On native we also log the error to crashlytics * Since the error was handled we need to manually tell crashlytics about it */ - crashlytics().log(`errorInfo: ${errorInfo}`); + crashlytics().log(`errorInfo: ${JSON.stringify(errorInfo)}`); crashlytics().recordError(error); }; diff --git a/tests/unit/loginTest.js b/tests/unit/loginTest.js index 02425b6dfa68..615790e4b38a 100644 --- a/tests/unit/loginTest.js +++ b/tests/unit/loginTest.js @@ -9,7 +9,9 @@ import React from 'react'; import renderer from 'react-test-renderer'; import App from '../../src/App'; -jest.mock('@react-native-firebase/crashlytics', () => ({ +/* uses and we need to mock the imported crashlytics module +* due to an error that happens otherwise https://github.com/invertase/react-native-firebase/issues/2475 */ +jest.mock('@react-native-firebase/crashlytics', () => () => ({ log: jest.fn(), recordError: jest.fn(), }));