-
-
Notifications
You must be signed in to change notification settings - Fork 338
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
feat: Allow disabling native SDK initialization but still use it #1259
Conversation
|
src/js/options.ts
Outdated
/** Initializes the native SDK on init. | ||
* Set this to `false` if you have an existing native SDK and don't want to re-initialize. | ||
* @default true | ||
*/ | ||
shouldInitializeNativeSdk?: boolean; |
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.
what are the edge cases of such flag?
eg 1. RN SDK captures an event, Native SDKs are not inited.
2. RN sets up X DSN but events go to Y DSN etc...
not against it, but we'll need to figure out such edge cases and write them down.
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.
Yes I agree, should mention any possible side effects and edge cases, and also need to document this clearly. Thanks for the catch!
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.
Being the author of the initial feature request, I'd like to add my 2 cents: Having the only source for DSN/environment etc would be a better option, so that I don't need to worry about that in the JS side, since it'd've been already fully initialised and configured on the native side.
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.
@pioneer32 that would be the perfect solution, agree, but I'd say we'd go the other way around.
we'd sync the options from RN -> Native as we already do, which is let's say ~90+% of the use cases.
you could open an issue as a feature request and let's see if it gets upvoted, but I'd bet on the case described above.
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.
@marandaneto Thank you for clarification! My guess is shouldInitializeNativeSdk
sort of going to disable calling
sentry-react-native/ios/RNSentry.m
Line 67 in 97b16b7
[SentrySDK startWithOptionsObject:sentryOptions]; |
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
For reference, this came up in Flutter too already: getsentry/sentry-dart#255 (comment) |
📢 Type of change
📜 Description
Allows the user to disable the initialization of the Native SDK in the case that they already have one initialized themselves. We do this by adding the
shouldInitializeNativeSdk
key toReactNativeOptions
. This is not breaking as it will default totrue
.💡 Motivation and Context
User has an app that is extensively native iOS and the React Native section is only run way later: #1248
💚 How did you test it?
Wrote a new test in
wrapper.ts
.📝 Checklist
🔮 Next steps
Should add documentation on this option.