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.

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

fix(rnerrorhanlers): Don't crash if ErrorUtils are not available #2808

Merged
merged 10 commits into from
Feb 9, 2023

Conversation

andrew-schenk
Copy link
Contributor

Adds a try/catch guard to keep react-native-web from crashing due to a missing global object ErrorUtils

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

📜 Description

Add in a try/catch around ErrorUtils functionality

💡 Motivation and Context

This is to keep react-native-web from crashing. This can occur in a react-native project that is using the web version of storybook. Components and stories rendered in that environment will crash due to using ErrorUtils global object which does not exist in react-native-web. For iOS and Android consumers, the functionality would be unchanged.

💚 How did you test it?

In a storybook.js project.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled
  • All tests passing
  • No breaking changes

🔮 Next steps

Adds a try/catch guard to keep react-native-web from crashing due to a missing global object `ErrorUtils`
Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

Thank you for the PR. We would like to make react-native-web compatible if only such a small change is necessary, but I'm not sure if it's enough.

Could we rather than wrapping the code in try-catch, add a guard at the begging of the function checking if ErrorUtils exist? Linting will fail if console.log is used, you can replace it with logger.

if (!ErrorUtils) {
    logger.warn('ErrorUtils not found. Can be caused by different environment for example react-native-web.');
	return;
}

CI Build failed: https://github.com/getsentry/sentry-react-native/actions/runs/4107836949/jobs/7101832808

Are JS errors captured when the global handler is not set? Because we do not add the web global error handlers by default in the RN SDK.

The web handlers: https://github.com/getsentry/sentry-javascript/blob/2aa4e94b036675245290596884959e06dcced044/packages/browser/src/integrations/globalhandlers.ts#L24

@andrew-schenk
Copy link
Contributor Author

Thank you for the PR. We would like to make react-native-web compatible if only such a small change is necessary, but I'm not sure if it's enough.

Could we rather than wrapping the code in try-catch, add a guard at the begging of the function checking if ErrorUtils exist? Linting will fail if console.log is used, you can replace it with logger.

if (!ErrorUtils) {
    logger.warn('ErrorUtils not found. Can be caused by different environment for example react-native-web.');
	return;
}

CI Build failed: https://github.com/getsentry/sentry-react-native/actions/runs/4107836949/jobs/7101832808

Are JS errors captured when the global handler is not set? Because we do not add the web global error handlers by default in the RN SDK.

The web handlers: https://github.com/getsentry/sentry-javascript/blob/2aa4e94b036675245290596884959e06dcced044/packages/browser/src/integrations/globalhandlers.ts#L24

if (!ErrorUtils) {

will still throw an undefined error in react-native-web. Switching it up to this works.

if(!global.ErrorUtils) {

In terms of making this library react-native-web compatible. I would say more work would need to be done. This change only makes Sentry fail silently which is helpful for my use case of putting RN components into a RNW storybook.

@krystofwoldrich
Copy link
Member

@andrew-schenk I've made some small changes, so ts and lint don't complain. Like that it would work for you?

Copy link
Member

@krystofwoldrich krystofwoldrich left a comment

Choose a reason for hiding this comment

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

LGTM 🚀
I've tested the cahnges with the repo sample app.
CI fails because it's running on a fork.

@krystofwoldrich krystofwoldrich changed the title Update reactnativeerrorhandlers.ts fix(rnerrorhanlers): Don't crash if ErrorUtils are not available Feb 9, 2023
@krystofwoldrich krystofwoldrich merged commit 58a3c6f into getsentry:main Feb 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants