Skip to content
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: Sync RN scope to native #902

Merged
merged 31 commits into from
Jun 5, 2020
Merged

feat: Sync RN scope to native #902

merged 31 commits into from
Jun 5, 2020

Conversation

jennmueng
Copy link
Member

@jennmueng jennmueng commented Jun 3, 2020

📢 Type of change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring

Fixes #694

📜 Description

Implemented a new class ReactNativeScope extends Scope that calls native scope setters equivalent methods on every call to setUser, addBreadcrumb, setExtra, setExtras, setTag, setTags, setContext, clearBreadcrumbs.

💡 Motivation and Context

Syncing the React Native scope to native events would allow users to actually group them together and see all the breadcrumbs from React Native that led to that event in native.

💚 How did you test it?

Created extensive scope assignment tests in the sample app.

📝 Checklist

  • I reviewed submitted code
  • I added tests to verify changes
  • All tests passing

🔮 Next steps

Copy link
Member

@bruno-garcia bruno-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great to see this landing!
I realize it's a draft but left some comments to help with direction.

CHANGELOG.md Show resolved Hide resolved
android/src/main/java/io/sentry/RNSentryModule.java Outdated Show resolved Hide resolved
android/src/main/java/io/sentry/RNSentryModule.java Outdated Show resolved Hide resolved
android/src/main/java/io/sentry/RNSentryModule.java Outdated Show resolved Hide resolved
android/src/main/java/io/sentry/RNSentryModule.java Outdated Show resolved Hide resolved
android/src/main/java/io/sentry/RNSentryModule.java Outdated Show resolved Hide resolved
android/src/main/java/io/sentry/RNSentryModule.java Outdated Show resolved Hide resolved
src/js/sdk.ts Show resolved Hide resolved
@jennmueng jennmueng changed the title DRAFT: feat: Sync RN scope to native feat: Sync RN scope to native Jun 4, 2020
ios/RNSentry.m Outdated Show resolved Hide resolved
src/js/scope.ts Outdated Show resolved Hide resolved
Copy link
Member

@HazAT HazAT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Few nits left

src/js/scope.ts Outdated Show resolved Hide resolved
src/js/scope.ts Outdated Show resolved Hide resolved
src/js/wrapper.ts Outdated Show resolved Hide resolved
@jennmueng jennmueng merged commit 0ec9e20 into master Jun 5, 2020
@jennmueng jennmueng deleted the feat/native-synced-scope branch June 5, 2020 07:15
@cruzach
Copy link
Contributor

cruzach commented Jun 5, 2020

I think this may have introduced some breaking changes- should the enableNative setting be checked before trying to call into these native methods?

@jennmueng
Copy link
Member Author

@cruzach Yes it does look like not checking for enableNative is a breaking bug. Will see if I can get something out ASAP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Context values not getting attached to native crashes
4 participants