expo / sentry-expo

MIT License
202 stars 83 forks source link

Upgrade `@sentry/react-native` to version `5` #337

Closed krystofwoldrich closed 1 year ago

krystofwoldrich commented 1 year ago

Checklist

Why

Thanks to @jongbelegen for starting this upgrade.

After this PR sentry-expo will have all new features and fixes from sentry-react-native version 5.

How

This PR reflects the changes described in the migration guide https://docs.sentry.io/platforms/react-native/migration/#from-4x-to-5x

Test Plan

Tested only locally.

iOS example error: https://krystofs-org.sentry.io/share/issue/0d3845c495eb4f6e8ae1061661775d6a/ Android example error: https://krystofs-org.sentry.io/share/issue/5ef20a7f762b4a5b91f61b9e948e2f33/

KiwiKilian commented 1 year ago

Awesome PR, I can confirm it works, now also on Android with source maps. One minor problem, at least while installing from github source branch in our CI, is the react devDependency. You can reproduce by running npm install in your sentry-expo branch:

npm ERR! code ERESOLVE
npm ERR! ERESOLVE could not resolve
npm ERR! 
npm ERR! While resolving: react-native-web@0.18.10
npm ERR! Found: react@18.1.0
npm ERR! node_modules/react
npm ERR!   dev react@"18.1.0" from the root project
npm ERR!   peer react@">=16.13.1" from react-error-boundary@3.1.4
npm ERR!   node_modules/react-error-boundary
npm ERR!     react-error-boundary@"^3.1.0" from @testing-library/react-hooks@7.0.2
npm ERR!     node_modules/@testing-library/react-hooks
npm ERR!       @testing-library/react-hooks@"^7.0.1" from expo-module-scripts@3.0.4
npm ERR!       node_modules/expo-module-scripts
npm ERR!         dev expo-module-scripts@"3.0.4" from the root project
npm ERR!   9 more (react-native, react-native-web, ...)
npm ERR! 
npm ERR! Could not resolve dependency:
npm ERR! peer react-dom@"^17.0.2 || ^18.0.0" from react-native-web@0.18.10
npm ERR! node_modules/react-native-web
npm ERR!   dev react-native-web@"~0.18.9" from the root project
npm ERR! 
npm ERR! Conflicting peer dependency: react@18.2.0
npm ERR! node_modules/react
npm ERR!   peer react@"^18.2.0" from react-dom@18.2.0
npm ERR!   node_modules/react-dom
npm ERR!     peer react-dom@"^17.0.2 || ^18.0.0" from react-native-web@0.18.10
npm ERR!     node_modules/react-native-web
npm ERR!       dev react-native-web@"~0.18.9" from the root project
npm ERR! 
npm ERR! Fix the upstream dependency conflict, or retry
npm ERR! this command with --force or --legacy-peer-deps
npm ERR! to accept an incorrect (and potentially broken) dependency resolution.

So it might be good to upgrade react to 18.2.0 in the devDependecies, but it might only occur through the github install. Also react-native devDependency seems to be conflicting after react upgrade.

Also the latest changes are not present in plugin/build.

Hoping to see this merge soon, thanks!

krystofwoldrich commented 1 year ago

@KiwiKilian Thank you for trying this out. Happy to hear that source maps on Android work. That is awesome.

I've updated the dev deps and the latest plugin build.

krystofwoldrich commented 1 year ago

@kbrandwijk Since there are breaking changes in @sentry/react-native and also changes in the native modules so people upgrading to this can do OTA update.

Would it make sense to release these changes as v7.0.0?

jongbelegen commented 1 year ago

I've tested it, and source maps seem to work for both ios and Android.

Thanks for pushing this forward!

flodev commented 1 year ago

hi, I'd really like to test this out as well. I noticed that in sentry-expo 7 the Sentry.init({}) parameters have changed. Can you point me to the docs where the new params are described? Or Could you give me a hint how I should pass the DSN now that this parameter is not there anymore?

Thanks in advance :)

KiwiKilian commented 1 year ago

I didn't have to change anything to init. Our setup looks like this:

import * as Sentry from 'sentry-expo';
import * as SentryReactNative from '@sentry/react-native';
import Constants from 'expo-constants';

const SENTRY_ENVIRONMENT = Constants.expoConfig?.extra?.appVariant.toLowerCase();

export const routingInstrumentation = new SentryReactNative.ReactNavigationInstrumentation();

Sentry.init({
  enableInExpoDevelopment: false,
  dsn: '<your-dsn-here>',
  enabled: SENTRY_ENVIRONMENT !== 'development',
  environment: SENTRY_ENVIRONMENT,
  integrations: [
    new SentryReactNative.ReactNativeTracing({
      routingInstrumentation,
    }),
  ],
});