-
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
fix: add analytics peerDependency to remote-config, dynamic-links, in-app-messaging #4850
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/invertase/react-native-firebase/dmahqx0b9 |
Codecov Report
@@ Coverage Diff @@
## master #4850 +/- ##
==========================================
+ Coverage 88.66% 89.09% +0.44%
==========================================
Files 109 109
Lines 3712 3712
Branches 347 347
==========================================
+ Hits 3291 3307 +16
+ Misses 379 363 -16
Partials 42 42 |
I've asked for review (always helps!) but default stance is to merge and release in a couple days in absence of review |
…o one commit previously it only updated the app module, but now we have packages that depend on analytics after #4850 previously it did one commit per module, which was effectively a git log spam
@mikehardy for dynamic links the docs appear to say it is optional, see here |
Yeah, I was a bit iffy on that one - thanks for providing deeper information - you'll note from my link in the PR that no optionality is mentioned (https://firebase.google.com/docs/dynamic-links/ios/create#set-up-firebase-and-the-dynamic-links-sdk) - makes it hard to reason about! Also there may be platform differences between ios and android - hopefully not as I am not sure how I would express that when the peerDependency is for both. At any rate, based on your documentation I will remove the peer dependency on analytics from dynamic links |
Per @nfcampos analytics is actually optional for dynamic links This information is not mentioned in main documentation but present in some supporting docs #4850 (comment)
Per @nfcampos analytics is actually optional for dynamic links This information is not mentioned in main documentation but present in some supporting docs #4850 (comment)
Yeah the documentation is a bit of a maze |
Description
Turns out that analytics is actually required for certain modules, or you get startup crashes - #4821
When analytics is optional the documents say so, like crashlytics
But for these ones it is not listed as optional, and a crash was observed
Dynamic Links
Remote Config
In App Messaging
Related issues
Fixes #4821
Release Summary
This should be rebase merged so that each module get its own changelog entry, in my opinion, otherwise I'd use:
fix(dependencies): add required analytics peerDependency to in-app-messaging, remote-config, dynamic-links
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