Skip to content
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.

Sign up for GitHub

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

Log unhandled promise rejections to the server #46173

Merged
merged 3 commits into from
Jul 30, 2024
Merged

Log unhandled promise rejections to the server #46173

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4c8e2c2
Log unhandled promise rejections to the server
Kureev Jul 25, 2024
a932817
Improve error handling experience and add handlers to the native plat…
Kureev Jul 29, 2024
dafadb9
Revert ErrorBoundary .native counterpart
Kureev Jul 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 24 additions & 1 deletion src/components/ErrorBoundary/index.native.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import crashlytics from '@react-native-firebase/crashlytics';
import React from 'react';
import React, {useEffect} from 'react';
import Log from '@libs/Log';
import BaseErrorBoundary from './BaseErrorBoundary';
import type {BaseErrorBoundaryProps, LogError} from './types';
Expand All @@ -14,7 +14,30 @@ const logError: LogError = (errorMessage, error, errorInfo) => {
crashlytics().recordError(error);
};

const onUnhandledRejection = (event: PromiseRejectionEvent) => {
let rejection: unknown = event.reason;
if (event.reason instanceof Error) {
Log.alert(`Unhandled Promise Rejection: ${event.reason.message}\nStack: ${event.reason.stack}`, {}, false);
crashlytics().log(`errorInfo: ${event.reason.message}`);
crashlytics().recordError(event.reason);
return;
}

if (typeof event.reason === 'object' && event.reason !== null) {
rejection = JSON.stringify(event.reason);
}

Log.alert(`Unhandled Promise Rejection: ${String(rejection)}`, {}, false);
crashlytics().log(`errorInfo: ${String(rejection)}`);
crashlytics().recordError(new Error(String(rejection)));
};

function ErrorBoundary({errorMessage, children}: Omit<BaseErrorBoundaryProps, 'logError'>) {
// Log unhandled promise rejections to the server
useEffect(() => {
window.addEventListener('unhandledrejection', onUnhandledRejection);
return () => window.removeEventListener('unhandledrejection', onUnhandledRejection);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Android as well as iOS are not recognizing window:

Screenshot 2024-07-30 at 12 14 14 AM

Also if i build the apps, then they would build successfullly but do not open, they crash after sometime, can you reproduce that @Kureev ? let me know if you need any additional information thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, can reproduce it now (after adding code to the .native.tsx file), thank you for this report. Testing the fix atm. Will update this PR accordingly

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so, there is a slight issue with the fix here. It isn't so trivial to handle unhandled promise rejections in RN. It seems that right now React Native uses https://www.npmjs.com/package/promise package and in particular onUnhandled to integrate unhandled promise rejections into the LogBox:

That said, I can do the same, but it poses some risks if RN team changes the internal approach. Would you like me to go down this way?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We agreed to not to implement .native counterpart and keep the solution to web only: https://expensify.slack.com/archives/C05LX9D6E07/p1722343972517359?thread_ts=1721217121.843389&cid=C05LX9D6E07

}, []);
return (
<BaseErrorBoundary
errorMessage={errorMessage}
Expand Down
21 changes: 20 additions & 1 deletion src/components/ErrorBoundary/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, {useEffect} from 'react';
import Log from '@libs//Log';
import BaseErrorBoundary from './BaseErrorBoundary';
import type {BaseErrorBoundaryProps, LogError} from './types';
Expand All @@ -8,7 +8,26 @@ const logError: LogError = (errorMessage, error, errorInfo) => {
Log.alert(`${errorMessage} - ${error.message}`, {errorInfo}, false);
};

const onUnhandledRejection = (event: PromiseRejectionEvent) => {
let rejection: unknown = event.reason;
if (event.reason instanceof Error) {
Log.alert(`Unhandled Promise Rejection: ${event.reason.message}\nStack: ${event.reason.stack}`, {}, false);
return;
}

if (typeof event.reason === 'object' && event.reason !== null) {
rejection = JSON.stringify(event.reason);
}
Log.alert(`Unhandled Promise Rejection: ${String(rejection)}`, {}, false);
};

function ErrorBoundary({errorMessage, children}: Omit<BaseErrorBoundaryProps, 'logError'>) {
// Log unhandled promise rejections to the server
useEffect(() => {
window.addEventListener('unhandledrejection', onUnhandledRejection);
return () => window.removeEventListener('unhandledrejection', onUnhandledRejection);
}, []);

return (
<BaseErrorBoundary
errorMessage={errorMessage}
Expand Down
Loading