-
-
Notifications
You must be signed in to change notification settings - Fork 342
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(expo): Dynamically resolve default integrations based on platform #3465
Conversation
|
Android (legacy) Performance metrics 🚀
|
Android (new) Performance metrics 🚀
|
iOS (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.
The comments that I made also applies to the old behaviour so it's not blocking this PR, with that I am approving the PR :D
|
||
integrations.push(createReactNativeRewriteFrames()); | ||
|
||
if (options.enableNative) { |
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.
users can set this parameter to true, should we also check for notweb()
here?
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 thought about that too, probably isNativeAvailable
would be even better, since there can be the case when we are not on the web but native is not available.
But since the enableNative
was in the past enforcing I wanted to keep that.
if (options.enableNative) { | ||
integrations.push(new DeviceContext()); | ||
integrations.push(new ModulesLoader()); | ||
if (options.attachScreenshot) { |
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.
should we warn users if debug is enabled that attachScreenshot will not be enabled because it's not supported on the web environment?
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 see the point, I think it makes sense, but for now, I would leave the warning only for cases when we expected something to work and it doesn't. So in happy scenarios, there are no warnings.
📢 Type of change
📜 Description
Currently, the same default integrations are added for all types of builds. This causes multiple warnings and error logs during event processing. Although events are processed correctly this might confuse the SDK users.
Before:
After:
Note: The SDK Info warning will be removed in the next PR with Expo Modules implementation.
💚 How did you test it?
sample app
📝 Checklist
sendDefaultPII
is enabled🔮 Next steps