expo / sentry-expo

MIT License
202 stars 83 forks source link

Distribution not matching with new isEmbeddedLaunch flag from SDK 47 #317

Closed yonitou closed 1 year ago

yonitou commented 1 year ago

Summary

Hi,

I don't know if this is a bug but I prefer to create an issue :

For almost 2 weeks, after switching my expo app to Hermes I noticed that my source maps didn't work anymore on Sentry.

The error was the following (the source maps are correctly uploaded) :

Capture d’écran 2023-01-30 à 15 19 31

I started to search why does my distribution flag has changed since I didn't touch it for almost one year :)

Here's my Sentry.init function :

Sentry.init({
    dsn: Constants.manifest.extra.sentryDSN,
    debug: __DEV__,
    release: Constants.manifest.extra.OTA,
});

Here's my config from app.config.ts for the auto publish plugin :

{
    file: "sentry-expo/upload-sourcemaps",
        config: {
                        organization: "alvie",
                        project: sentryProject,
                        authToken: "XXXXXXXXXXX",
                        release: OTA,
            },
    },

I digged into the sentry-expo repository and found this getDist() function ?

function getDist(): string | null | undefined {
  if (Updates.isEmbeddedLaunch) {
    return MANIFEST.revisionId ? MANIFEST.version : `${Application.nativeBuildVersion}`;
  } else {
    return Updates.updateId;
  }
}

Looks like that expo is automatically taking the Updates.updateId instead of the expo app version as described on the expo documentation : "distribution [...]Expo defaults to using your version from app.json"

After changing my Sentry.init to the code below, everything worked well !

Sentry.init({
    dsn: Constants.manifest.extra.sentryDSN,
    dist: Constants.manifest.extra.version,
    debug: __DEV__,
    release: Constants.manifest.extra.OTA,
});

So maybe I missed something from the documentation or maybe the documentation should be updated to clarify that from now on, we need to manually set the dist flag from Sentry.init call to the the expo app version to match the one from app.json plugin.

I hope everything was clear

Thanks & have a good day

Managed or bare workflow? If you have ios/ or android/ directories in your project, the answer is bare!

managed

What platform(s) does this occur on?

Android, iOS

SDK Version (managed workflow only)

48

Environment

expo-env-info 1.0.5 environment info:
    System:
      OS: macOS 13.1
      Shell: 5.8.1 - /bin/zsh
    Binaries:
      Node: 18.12.1 - ~/.nvm/versions/node/v18.12.1/bin/node
      npm: 8.19.2 - ~/.nvm/versions/node/v18.12.1/bin/npm
    Managers:
      CocoaPods: 1.11.2 - /Users/yonitouboul/.rbenv/shims/pod
    SDKs:
      iOS SDK:
        Platforms: DriverKit 21.4, iOS 16.0, macOS 12.3, tvOS 16.0, watchOS 9.0
    IDEs:
      Android Studio: 2021.2 AI-212.5712.43.2112.8609683
      Xcode: 14.0.1/14A400 - /usr/bin/xcodebuild
    npmPackages:
      @expo/metro-config: 0.5.2 => 0.5.2 
      @expo/webpack-config: 0.17.4 => 0.17.4 
      babel-preset-expo: 9.2.2 => 9.2.2 
      expo: 47.0.9 => 47.0.9 
      react: 18.1.0 => 18.1.0 
      react-dom: 18.1.0 => 18.1.0 
      react-native: 0.70.5 => 0.70.5 
    npmGlobalPackages:
      eas-cli: 3.0.0
      expo-cli: 6.0.8
    Expo Workflow: managed

Reproducible demo or steps to reproduce from a blank project

  1. Create a new expo app
  2. Install sentry-expo
  3. Call Sentry.init in the root file with the den, release and debug config above
  4. Configure the post-publish hook as described above
  5. Release the app in internal distribution
  6. Look at the errors details
kbrandwijk commented 1 year ago

What version of expo-updates do you have in your project? If it doesn't have the isEmbeddedLaunch property yet, then that might explain why the logic is failing.

ken0x0a commented 1 year ago

https://github.com/expo/sentry-expo/blob/699b6f7150dd0d5a9a3f7d66e3f9cda134af6dc2/src/hooks/upload-sourcemaps.ts#L96

This doesn't match to Updates.updateId at

https://github.com/expo/sentry-expo/blob/699b6f7150dd0d5a9a3f7d66e3f9cda134af6dc2/src/sentry.ts#L31-L37

Updates.isEmbeddedLaunch is false after OTA.

So, how about change https://github.com/expo/sentry-expo/blob/699b6f7150dd0d5a9a3f7d66e3f9cda134af6dc2/src/hooks/upload-sourcemaps.ts#L96 to

distribution: manifest.releaseId || distribution || env.SENTRY_DIST || manifest.version,

On my environment, distribution was undefined.

I'm using expo publish command.

arekm213 commented 1 year ago

I had similar issue on "react-native": "0.71.4", "sentry-expo": "6.1.1", "@sentry/react-native": "4.13.0",

In bare workflow, we use expo publish for OTA updates and @ken0x0a solution worked Thanks a lot!

Edit: Unfortunately it didnt work for Android, iOS works good

github-actions[bot] commented 1 year ago

This issue is stale because it has been open for 60 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

krystofwoldrich commented 1 year ago

This doesn't seem to be solved, let's not close the issue.

kbrandwijk commented 1 year ago

The value set by the script for dist is based on the current logic that applies to EAS Update. This might be incompatible with earlier defaults to expo publish, however Classic Updates have sunsetted, and we're not introducing changes to ensure continued compatibility with it. You can specify dist manually in your init call, as you've shown above, if you're still using Classic Updates. As this workaround is available, I'm going to consider this issue resolved.