-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
fix: Fix iOS build issue on Xcode 16.3 #15045
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
base: main
Are you sure you want to change the base?
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. Ignoring alerts on:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fixed my iOS builds (using xcode 16.3)
@@ -285,7 +285,7 @@ function startSpan<T>( | |||
op: op || OP_DEFAULT, | |||
// This needs to be parentSpan once we have the withIsolatedScope implementation in place in the Sentry SDK for React Native | |||
// Reference PR that updates @sentry/react-native: https://github.com/getsentry/sentry-react-native/pull/3895 | |||
parentSpanId: parentSpan?.spanId, | |||
parentSpan, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Takes an entire span now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know where this change is documented? I'm having a bit of trouble following.
I see something about Span mentioned here: https://docs.sentry.io/platforms/javascript/migration/v7-to-v8/#removal-of-span-class-export-from-sdk-packages
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, the removed the Span
class from exports, but they are still exporting the type.
I found a dedicated page about the trace API changes. There is a section about how to set a parent span: https://docs.sentry.io/platforms/javascript/migration/v7-to-v8/v8-new-performance-api/#creating-a-child-span-of-a-specific-span
They suggest a different method (Sentry.withActiveSpan
). It's unclear how this differs from setting the parentSpan
option 🤔.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why this option wasn't referenced in the migration docs, but I found it elsewhere in their docs here: https://docs.sentry.io/platforms/javascript/tracing/instrumentation/#starting-spans-as-children-of-a-specific-span
This suggests what we're doing here is correct, no need to use withActiveSpan
@@ -373,11 +373,3 @@ function tryCatchMaybePromise<T>( | |||
|
|||
return undefined; | |||
} | |||
|
|||
function sentrySetMeasurement( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need util. Can just call setMeasurement directly
patches/@sentry+core+8.54.0.patch
Outdated
+ * Patch to enforce better stack tracing using Error - https://docs.sentry.io/platforms/javascript/usage/#capturing-errors | ||
*/ | ||
-export declare function captureException(exception: unknown, hint?: ExclusiveEventHintOrCaptureContext): string; | ||
+export declare function captureException(exception: Error, hint?: ExclusiveEventHintOrCaptureContext): string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure better stack trace with Error
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed that we should be passing errors to captureException
, but why do we need this patch?
If we really want this type checked more strictly, we can define our own captureException
that calls Sentry's. Typically we avoid patches except as a last resort, as they make updates significantly more difficult.
@SocketSecurity ignore-all |
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmation changes LGTM
package.json
Outdated
@@ -254,7 +254,7 @@ | |||
"@reown/walletkit": "^1.2.3", | |||
"@segment/sovran-react-native": "^1.0.4", | |||
"@sentry/integrations": "6.3.1", | |||
"@sentry/react-native": "^5.33.0", | |||
"@sentry/react-native": "~6.10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why limit the range just to this minor version? Is there a breaking change in version 6.11?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This specific version was auto updated using Expo CLI via npx expo install --fix
. It's part of Expo's workflow for version validation. My guess is that they have a manifest somewhere that keeps track of this. This was just their suggested version but I don't see why we need to limit to the minor version as well. I can update this in a bit
package.json
Outdated
@@ -254,7 +254,7 @@ | |||
"@reown/walletkit": "^1.2.3", | |||
"@segment/sovran-react-native": "^1.0.4", | |||
"@sentry/integrations": "6.3.1", | |||
"@sentry/react-native": "^5.33.0", | |||
"@sentry/react-native": "~6.10.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This breaking change made in v6 seems to impact us: https://docs.sentry.io/platforms/react-native/migration/v5-to-v6/#updated-behavior-of-tracepropagationtargets-in-react-native
Effectively it means tracing headers are now added on all network requests made outside of the webview, whereas previously they were not. We may want to explicitly set tracePropagationTargets
to the v5 default value to avoid any unintended changes here
package.json
Outdated
@@ -427,7 +427,7 @@ | |||
"@open-rpc/test-coverage": "^2.2.2", | |||
"@react-native/metro-config": "0.76.9", | |||
"@rpii/wdio-html-reporter": "^7.7.1", | |||
"@sentry/types": "^7.117.0", | |||
"@sentry/core": "8.54.0", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The update to v8 includes some breaking changes that haven't been dealt with yet. I'm surprised it isn't throwing an error 🤔
See here: https://docs.sentry.io/platforms/javascript/migration/v7-to-v8/#removal-of-class-based-integrations
We're still using two class-based integrations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the integrations aren't throwing an error because we're importing them from the integrations package, not from core. And the integrations package was not updated in this PR.
Still seems advisable to update it though, at least according to the migration doc. It's unclear whether there are any consequences to using outdated integrations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like some breaking changes are not yet handled
app/util/trace.ts
Outdated
// This needs to be parentSpan once we have the withIsolatedScope implementation in place in the Sentry SDK for React Native | ||
// Reference PR that updates @sentry/react-native: https://github.com/getsentry/sentry-react-native/pull/3895 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is now obsolete
// This needs to be parentSpan once we have the withIsolatedScope implementation in place in the Sentry SDK for React Native | |
// Reference PR that updates @sentry/react-native: https://github.com/getsentry/sentry-react-native/pull/3895 |
|
Description
Fix iOS build issues on Xcode 16.3 by bumping Sentry and Expo deps. This also updates pods repo so a few other minor version bumps will exist in Podfile.lock as well
Related issues
Fixes:
Manual testing steps
How it was tested: Both Android and iOS apps should run after running
yarn start:ios
oryarn start:android
Screenshots/Recordings
Before
Individuals began experiencing iOS build issues with Xcode 16.3 as reported here - https://consensys.slack.com/archives/C02U025CVU4/p1746041871082239. The fix was to bump Expo SDK deps as well as Sentry.
Expo SDK deps - https://expo.dev/changelog/xcode-16-3-patches
Sentry issue - https://github.com/getsentry/sentry-cocoa/releases/tag/8.46.0 (we bumped to stable 8.48.0 with pod repo update)
After
Pre-merge author checklist
Pre-merge reviewer checklist