-
-
Notifications
You must be signed in to change notification settings - Fork 357
fix(scope): Hooks into Global and Isolations Scopes for Sync with the native scope #3911
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
Conversation
…w/fix-native-scope-sync
iOS (legacy) Performance metrics 🚀
|
Android (new) Performance metrics 🚀
|
lucas-zimerman
left a comment
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 asked some minor changes, but other than that it looks good for merge, good job on the solution chosen here!
lucas-zimerman
left a comment
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.
Actually, there is one thing that is bothering me, with your implementation, doesn't it have a chance to break if users uses minification on their code? in that case, the name setTag may change but not the string literal setTag.
|
I've check the minified output of the Metro bundler and it correctly recognizes the dynamic access to the properties and does not minify the scope. We dynamically access scope properties in the core as well, so even if changed in RN the scope would not be minified. |
Android (legacy) Performance metrics 🚀
|
lucas-zimerman
left a comment
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.
All looks good from my part. We should wait for the other reviewer before merging.
mydea
left a comment
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.
this makes sense to me 👍
📢 Type of change
📜 Description
This PR adds necessary scope changes to enables hybrid SDKs (React Native) to synchronize their scopes to the native SDKs which are using the Hub and Scopes architecture.
The following variants were considered.
Change static RN SDK APIs to operate on Global Scope
This will cause all integration from JS Core to stop working as they do not use the RN SDK APIs.
Add Default Scope which all SDK APIs will use
This adds new scope to the mix and increases complexity.
Set Global and Isolation scope as one reference.
Due to default falls back any interaction with the static APIs before Sentry.init will break the sync. The isolation scope will be created on first call before RN setup.
Use scope change listener
Not suitable for sync as it returns whole scope, comparing diff is too expensive.
Hook into scope changes
This work with API calls before init, the date before init won't be sync the same as in the previous implementation. We patch our own object, in the future we can use listeners for individual changes.
💚 How did you test it?
sample app, unit tests
📝 Checklist
sendDefaultPIIis enabled#skip-changelog