-
-
Notifications
You must be signed in to change notification settings - Fork 337
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 issue with Sentry React Native SDK's enableNative variable #3093
Fix issue with Sentry React Native SDK's enableNative variable #3093
Conversation
This commit addresses a problem with the Sentry React Native SDK where the this.enableNative variable was not correctly reinitialized to true after reinitializing the SDK. The issue arose when calling the closeNativeSdk() method, which set this.enableNative to false. Without reinitializing it to true, subsequent reinitializations of the SDK would fail to enable the native SDK functionality. To fix this, I added a line in the initNativeSdk function to explicitly set this.enableNative = true after reinitializing the native SDK. This ensures that the native SDK is correctly enabled when reinitializing. This change resolves the problem and ensures that the enableNative variable is properly managed during SDK reinitialization, allowing for the correct functioning of the Sentry React Native SDK.
Hi, Since this is mainly a matter of handling the Here sentry-react-native/test/wrapper.test.ts Lines 133 to 134 in d187887
test('does not initialize with autoInitializeNativeSdk: false', async () => {
+ NATIVE.enableNative = false;
logger.warn = jest.fn(); and sentry-react-native/test/wrapper.test.ts Lines 490 to 491 in d187887
test('closeNativeSdk calls native bridge', async () => {
+ NATIVE.enableNative = true;
await NATIVE.closeNativeSdk(); |
@krystofwoldrich sure! Just did. Thank you! |
@mateioprea It's looking good, you can ignore the e2e tests falling. But the unit test failed too. The flag will also need to be set here https://github.com/getsentry/sentry-react-native/pull/3093/files#diff-80149da618e89fad93a582eabad62eb682bd2774a2fdb209b84001813708f0f8R183 |
@krystofwoldrich thank you! I just pushed a fix for that. |
@krystofwoldrich i don't understand why the e2e tests are failing now. everything should be good. |
@mateioprea The e2e build tests fail because the auth tokens are missing for the community PR. The tests fail on uploading the source maps. But that is okay. |
ok. perfect. so is this something else that I should this on this PR? |
@mateioprea The for the follow-up. Yes, one of the unit tests in describe('nativeCrash', () => {
test('calls NativeModules crash', () => {
const RN: MockedReactNative = require('react-native');
const client = new ReactNativeClient({
...DEFAULT_OPTIONS,
+ dsn: EXAMPLE_DSN, And as last, a changelog update is needed. ## Unreleased
+ ### Fixes
+
+ - Native wrapper methods don't throw disabled error after re-initializing [#3093](https://github.com/getsentry/sentry-react-native/pull/3093)
+
### Dependencies |
@krystofwoldrich thank you for guidance. I've updated the files accordingly. |
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.
Thank you for the PR and the work. 🚀
Thank you @krystofwoldrich. Have a great day! |
📢 Type of change
📜 Description
This commit addresses a problem with the Sentry React Native SDK where the this.enableNative variable was not correctly reinitialized to true after reinitializing the SDK. The issue arose when calling the
closeNativeSdk()
method, which setthis.enableNative
tofalse
. Without reinitializing it to true, subsequent reinitializations of the SDK would prevent stopping the SDK afterwards.To fix this, I added a line in the initNativeSdk function to explicitly set
this.enableNative = true
after reinitializing the native SDK.This change resolves the problem and ensures that the enableNative variable is properly managed during SDK reinitialization, allowing for the correct functioning of the Sentry React Native SDK.
💡 Motivation and Context
The motivation behind this change is to ensure that the enableNative variable is correctly managed during SDK reinitialization, allowing for the proper functioning of the Sentry React Native SDK.
This change fixes the issue and ensures that the enableNative variable is reset to true after reinitializing the native SDK, thereby enabling the native SDK functionality as intended.
💚 How did you test it?
I conducted testing of this change on my local application to ensure its effectiveness. The following steps were taken during the testing process:
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps