-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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(crashlytics, native): add non-fatal exception logger for 3rd party native code use #5015
feat(crashlytics, native): add non-fatal exception logger for 3rd party native code use #5015
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/invertase/react-native-firebase/6wnkKf3G7mAA6BQULf7pmxDmJ1iT |
Codecov Report
@@ Coverage Diff @@
## master #5015 +/- ##
=======================================
Coverage 89.09% 89.09%
=======================================
Files 109 109
Lines 3728 3728
Branches 350 350
=======================================
Hits 3321 3321
Misses 365 365
Partials 42 42 |
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 am not sure why iOS E2E CI is failing, but it can't be related to this, as the compile works and there are no E2E tests that use this. It failed for an unrelated PR as well. I will work to resolve that
This PR in itself looks good to go though
@powerserg17-bunch I think we just need the CLA signed then I can merge this! https://cla-assistant.io/invertase/react-native-firebase?pullRequest=5015 |
Signed the CLA, thank you Mike! |
It turns out there was a valid iOS build problem. And our CI setup was hiding them - this bit another PR just now as well.
I have fixed the thing that causes iOS build failures in CI to silently fail so this won't happen for future PRs, but you'll have to fix this one manually and check visually with |
@mikehardy thanks for the information. Seems like it builds now. |
68d2dd9
to
6bd5dc3
Compare
I rebased it over master to pull in the already-merged CI pipefail fixes, if/when CI finishes chewing on it, it should be good to merge... |
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 and passes CI (finally! it was a little flaky), ready for merge
This is out as of v11.1.0 - please test and if there's anything wrong let us know - cheers! |
Description
This change allows to log non-fatal exception in Firebase Crashlytics, handled on the native side.
Related issues
Discussion
Release Summary
Checklist
Android
iOS
e2e
tests added or updated inpackages/\*\*/e2e
jest
tests added or updated inpackages/\*\*/__tests__
🔥
Test Plan
Think
react-native-firebase
is great? Please consider supporting the project with any of the below:React Native Firebase
andInvertase
on Twitter