-
-
Notifications
You must be signed in to change notification settings - Fork 337
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
fix(ios): tracesSampler becomes NSNull in iOS and the app cannot be started #1872
Conversation
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.
LGTM. I think we could add a layer of abstraction to add tests in Swift for the whole bridge. I can help you with that. Of course not in this PR. What do you think?
NSMutableDictionary * mutableOptions =[options mutableCopy]; | ||
[mutableOptions setValue:beforeSend forKey:@"beforeSend"]; |
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.
l
:
NSMutableDictionary * mutableOptions =[options mutableCopy]; | |
[mutableOptions setValue:beforeSend forKey:@"beforeSend"]; | |
NSMutableDictionary * mutableOptions = [options mutableCopy]; | |
mutableOptions[@"beforeSend"] = beforeSend; |
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'd rather keep as it is and change in another PR, there are more places in this class doing the same way
yep, I intend to do that after cleaning up all the JS issues first, also for Android, will hook you up when we are there, thanks. |
Agree, I created an issue so we don't forget about it #1874. |
📢 Type of change
📜 Description
💡 Motivation and Context
Closes #1864
💚 How did you test it?
running with sampler
📝 Checklist
🔮 Next steps