-
-
Couldn't load subscription status.
- Fork 353
feat(expo): Add RNSentrySDK APIs support to @sentry/react-native/expo plugin #4633
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: capture-app-start-errors
Are you sure you want to change the base?
feat(expo): Add RNSentrySDK APIs support to @sentry/react-native/expo plugin #4633
Conversation
|
iOS (legacy) Performance metrics 🚀
|
iOS (new) Performance metrics 🚀
|
Android (legacy) Performance metrics 🚀
|
Android (new) Performance metrics 🚀
|
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.
Overall the PR looks good and thank you for the tests!
I still think we could fine tune a bit more the android part and after it we could get ready for merge
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
|
Note: The Build & Test / Type Check Typescript 3.8 (pull_request) failure should be fixed when we merge #4673 from |
Co-authored-by: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com>
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.
I have re-checked the PR and overall, I only found a nit on a string. After fixing it and if @krystofwoldrich doesn't find anything else, LGTM!
Co-authored-by: LucasZF <lucas-zimerman1@hotmail.com>
Co-authored-by: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com>
|
I'm currently updating the base branch with the latest main since there was quite a lot of development that was not synced to the feature branch. |
Thank you for the heads up @krystofwoldrich 🙇 |
Co-authored-by: Krystof Woldrich <31292499+krystofwoldrich@users.noreply.github.com>
…getsentry/sentry-react-native into antonis/4625-expo-useNativeInit
2363742 to
588ba6d
Compare
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 fixes. 🚀 It looks great!
|
|
||
| ### Features | ||
|
|
||
| - Add RNSentrySDK APIs support to @sentry/react-native/expo plugin ([#4633](https://github.com/getsentry/sentry-react-native/pull/4633)) |
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.
Nit. It would be nice to include an example code snippet and a small summary of what will the flag do.
Co-authored-by: Antonis Lilis <antonis.lilis@gmail.com>
| ### Features | ||
|
|
||
| - Add RNSentrySDK APIs support to @sentry/react-native/expo plugin ([#4633](https://github.com/getsentry/sentry-react-native/pull/4633)) |
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 changelog entry seems to be part of an already released section
## 6.15.1.
Consider moving the entry to the## Unreleasedsection, please.
|
@sentry review |
| config.modResults.contents = config.modResults.contents.replace( | ||
| /(super\.onCreate\(\)[;\n]*)([ \t]*)/, | ||
| `$1\n$2RNSentrySDK.init(this);\n$2`, | ||
| ); |
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 regex pattern /(super\.onCreate\(\)[;\n]*)([ \t]*)/ may not handle all edge cases properly. For example, if there are multiple spaces or tabs before the next line, or if there are comments immediately after super.onCreate(), the insertion might not be placed correctly. Consider adding a test case for files with unusual formatting.
Did we get this right? 👍 / 👎 to inform future reviews.
| /(func application\([^)]*\) -> Bool \{)\s*\n(\s*)/s, | ||
| `$1\n$2RNSentrySDK.start()\n$2`, | ||
| ); |
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 regex pattern /(func application\([^)]*\) -> Bool \{)\s*\n(\s*)/s may fail to match if the function signature is split across multiple lines or has different whitespace patterns. Consider making the pattern more robust to handle multi-line function signatures:
/(func application\([^)]*\)\s*->\s*Bool\s*\{)\s*\n(\s*)/sDid we get this right? 👍 / 👎 to inform future reviews.
| config.modResults.contents = config.modResults.contents.replace( | ||
| /(- \(BOOL\)application:[\s\S]*?didFinishLaunchingWithOptions:[\s\S]*?\{\n)(\s*)/s, | ||
| `$1$2[RNSentrySDK start];\n$2`, |
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 regex pattern /(- \(BOOL\)application:[\s\S]*?didFinishLaunchingWithOptions:[\s\S]*?\{\n)(\s*)/s is overly greedy and could potentially match unintended content if there are multiple methods with similar signatures. Consider making it more specific by including the method return or limiting the character classes:
/(- \(BOOL\)application:\(UIApplication \*\)\w+\s+didFinishLaunchingWithOptions:\(NSDictionary \*\)\w+\s*\{\n)(\s*)/sDid we get this right? 👍 / 👎 to inform future reviews.
|
|
||
| it('should insert import statements only once in an Swift project', async () => { | ||
| config.modResults.contents = | ||
| 'import UIKit\nimport RNSentrySDK\n\noverride func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]? = nil) -> Bool {'; | ||
|
|
||
| const result = (await modifyAppDelegate(config)) as MockedExpoConfig; | ||
|
|
||
| const importCount = (result.modResults.contents.match(/import RNSentrySDK/g) || []).length; |
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 test name and logic are inconsistent. The test checks for 'import RNSentrySDK' but the actual implementation imports 'import RNSentry'. This test should verify the correct import statement.
The expected import should be import RNSentry not import RNSentrySDK.
| it('should insert import statements only once in an Swift project', async () => { | |
| config.modResults.contents = | |
| 'import UIKit\nimport RNSentrySDK\n\noverride func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]? = nil) -> Bool {'; | |
| const result = (await modifyAppDelegate(config)) as MockedExpoConfig; | |
| const importCount = (result.modResults.contents.match(/import RNSentrySDK/g) || []).length; | |
| it('should insert import statements only once in an Swift project', async () => { | |
| config.modResults.contents = | |
| 'import UIKit\nimport RNSentry\n\noverride func application(_ application: UIApplication, didFinishLaunchingWithOptions launchOptions: [UIApplication.LaunchOptionsKey: Any]? = nil) -> Bool {'; | |
| const result = (await modifyAppDelegate(config)) as MockedExpoConfig; | |
| const importCount = (result.modResults.contents.match(/import RNSentry/g) || []).length; | |
| expect(importCount).toBe(1); | |
| }); |
Did we get this right? 👍 / 👎 to inform future reviews.
| if (config.modResults.contents === originalContents) { | ||
| warnOnce(`Failed to insert 'RNSentrySDK.init' in '${fileName}'.`); | ||
| } else if (!config.modResults.contents.includes('import io.sentry.react.RNSentrySDK;')) { | ||
| // Insert import statement after package declaration | ||
| config.modResults.contents = config.modResults.contents.replace( | ||
| /(package .*;\n\n?)/, | ||
| `$1import io.sentry.react.RNSentrySDK;\n`, | ||
| ); | ||
| } | ||
| } else if (config.modResults.language === 'kt') { | ||
| // Add RNSentrySDK.init | ||
| const originalContents = config.modResults.contents; |
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.
When the regex replacement fails and the contents remain unchanged, only a warning is logged but the import statement might still be added in the else block. This could result in an orphaned import statement without the corresponding initialization call. Consider checking if the replacement was successful before adding the import statement.
Did we get this right? 👍 / 👎 to inform future reviews.
| ); | ||
| if (config.modResults.contents === originalContents) { | ||
| warnOnce(`Failed to insert 'RNSentrySDK.start()' in '${fileName}'.`); | ||
| } else if (!config.modResults.contents.includes('import RNSentry')) { | ||
| // Insert import statement after UIKit import | ||
| config.modResults.contents = config.modResults.contents.replace(/(import UIKit\n)/, `$1import RNSentry\n`); | ||
| } |
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.
When the regex replacement fails and the contents remain unchanged, only a warning is logged but the import statement might still be added in the else block. This could result in an orphaned import statement without the corresponding initialization call. Consider checking if the replacement was successful before adding the import statement.
Did we get this right? 👍 / 👎 to inform future reviews.
📢 Type of change
Based on feat: Capture app start errors before JS
📜 Description
Adds RNSentrySDK APIs support to @sentry/react-native/expo plugin by importing sentry and adding injecting
RNSentrySDK.init/startin the Android MainApplication (Kotlin or Java) or AppDelegate (Objective-C or Swift).This feature is opt-out to enable it set
useNativeInittotruein your@sentry/react-native/expoplugin configuration.💡 Motivation and Context
Fixes #4625
💚 How did you test it?
CI, Manual
📝 Checklist
sendDefaultPIIis enabled🔮 Next steps