-
Notifications
You must be signed in to change notification settings - Fork 58
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
Add Sentry SDK #3629
Add Sentry SDK #3629
Conversation
Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job! |
# Conflicts: # package-lock.json
# Conflicts: # package-lock.json
metro.config.js
Outdated
const moduleFolderPnpm = path.join( | ||
process.cwd(), | ||
`./jetpack/node_modules/.pnpm/node_modules/${ name }` | ||
); | ||
|
||
// pnpm uses symlinks so, let's find the target | ||
const symlinkTarget = fs.readlinkSync( moduleFolderPnpm ); | ||
if ( fs.existsSync( moduleFolderPnpm ) ) { |
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 noticed that the dependencies from the package.json
file of Gutenberg Mobile were being resolved using the Jetpack path, so I added this condition to prevent it.
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, is that related to the Sentry work here, or are you referring to "normal" dependencies? If the latter, perhaps we should bring that fix in its own PR and merge it sooner?
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'm referring to normal dependencies (the ones we have here).
If the latter, perhaps we should bring that fix in its own PR and merge it sooner?
Sure! I'll open a new PR for this 👍 .
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.
Here is the PR where I'm addressing this issue.
@@ -105,6 +105,7 @@ | |||
"version": "npm run bundle && git add -A bundle" | |||
}, | |||
"dependencies": { | |||
"email-validator": "2.0.4" | |||
"email-validator": "2.0.4", | |||
"@sentry/react-native": "2.4.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.
I had to use this specific version of the package because we have to match the dependency version of Sentry iOS SDK (reference in version 2.4.2
) with the one we have in Automattic-Tracks-iOS
.
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.
Tested via WordPress/gutenberg#32768 (review)
The code changes LGTM too 🎉
# Conflicts: # metro.config.js # package-lock.json # package.json
gutenberg
PR: WordPress/gutenberg#32768WordPress-iOS
PR: wordpress-mobile/WordPress-iOS#16700Automattic-Tracks-iOS
PR: Automattic/Automattic-Tracks-iOS#183This PR adds the Sentry React native SDK and customize the initialization to accomplish the following items:
beforeSend
callback to attach the same scope defined in the main apps and prevent sending events as it's determined in the main apps (code reference).NOTE: The Sentry SDK will be only enabled on iOS.
To test:
Follow the testing instructions from
gutenberg
PR: WordPress/gutenberg#32768.PR submission checklist: