fix(ios, analytics): remove now-optional analytics dependencies

[dlockwo]

Description

Removed remaining analytics dependencies from IOS podspecs. Analytics cannot be used in the IOS apps in the kids category. FirebaseCore no longer pulls analytics. Analytics can still be added separately and interop correctly with these modules.

Related issues

Fixes #4072

Release Summary

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Yes
• My change supports the following platforms;
  • Android
  • iOS
• My change includes tests;
  • e2e tests added or updated in packages/\*\*/e2e
  • jest tests added or updated in packages/\*\*/__tests__
• I have updated TypeScript types that are affected by my change.
• This is a breaking change;
  • Yes
  • No

Test Plan

Ran jest and e2e tests successfully.

---

Think react-native-firebase is great? Please consider supporting the project with any of the below:

• 👉 Star this repo on GitHub ⭐️
• 👉 Follow React Native Firebase and Invertase on Twitter

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/cx5dilcto
✅ Preview: https://react-native-firebase-git-fork-dlockwo-master.invertase.vercel.app

[mikehardy]

Oh - there were 2 more of these than I was thinking. I remember analyzing it on the linked issue though, this is still correct I think and should work just fine.

I am approving provisionally with one question: Did you build your app with these modules but without Analytics to make sure it worked? If not, that's the final test pre-merge - a quick throwaway build (like from https://github.com/mikehardy/rnfbdemo/blob/master/make-demo.sh but without this line https://github.com/mikehardy/rnfbdemo/blob/eda24a71990e95e8a91b17b27b0cc1417031808b/make-demo.sh#L79)

[mikehardy]

(the reason I ask for the build test is because the e2e test pulls them all in, so it won't verify the "analytics is not there" build case)

[dlockwo]

I cloned the rfnbdemo repo, ran the script to build the project, modified the 4 podspecs and App.js to exclude analytics, cleared the pods and then rebuilt. I confirmed that analytics was not present and that the app ran correctly. Note I also had to remove Crashlytics as it references Firebase/Core which references Firebase/Analytics. (This appears to be correct as the Crashlytics podspec in the IOS SDK still references Firebase/Core).

[mikehardy]

Very interesting result about Crashlytics! @UberMC has a fork that did something similar but on an older RNFB version - when Core was still propagated everywhere, before it was pulled up to just rnfb/app and switched to CoreOnly

I can't think of a way to switch Crashlytics from Core to CoreOnly though except via some Podfile-fu postinstall. Have you tried (just to see if it would work?) to edit FirebaseCrashlytics to be CoreOnly instead of Core? My Podfile-fu is weak unfortunately - I know things are possible but it takes me ages to express them in ruby with the unfamiliar object graph in Podfiles

This is already a great improvement of course, I'll merge this right away, but the result of that test would shape what sort of issue to open upstream

[mikehardy]

Perhaps the inimitable @wilhuff would know - or know the right people - with regard to the FirebaseCrashlytics.podspec and whether it can go CoreOnly as a dependency vs Core, enabling Crashlytics use in an app for Kids on iOS

[mikehardy]

This just went live:

 - @react-native-firebase/dynamic-links@7.4.3
 - @react-native-firebase/in-app-messaging@7.3.3
 - @react-native-firebase/messaging@7.7.3
 - @react-native-firebase/remote-config@8.1.4
 - react-native-firebase-tests@8.3.6

[ryanwilson]

👋 hey folks - Ryan from the Firebase iOS team here (@wilhuff pointed this thread out).

The naming here is a bit confusing with some overloaded terms, but Crashlytics does not have a hard dependency on Analytics and shouldn't pull in the framework. We have the Firebase "umbrella" pod which is used for user convenience (allowing for import Firebase) with each product as a subspec, plus the actual frameworks as their own standalone pods. Breaking it down:

• Firebase/Core: Umbrella pod, Core subspec. This includes FirebaseCore and includes Firebase Analytics.
• Firebase/CoreOnly: Umbrella pod, CoreOnly subspec. This only includes FirebaseCore, not Analytics.
• FirebaseCore (no slash): Standalone pod - contains all the code for the FirebaseCore framework. Does not include Analytics.

The FirebaseCrashlytics pod depends on FirebaseCore which is correct, not pulling in Analytics, and the Firebase/Crashlytics umbrella pod subspec depends on Firebase/CoreOnly (which is probably redundant, we should evaluate removing it).

Firebase/Core is really only there for historical purposes and to not breaking existing installations - previously we recommended that Firebase/Core was added to your Podfile in the setup instructions but we've changed to say explicitly include Firebase/Analytics if you want Analytics.

Let me know if that makes sense or if there's something else I'm missing that's pulling in FirebaseAnalytics unintentionally - it's definitely not the intention to have it as a hard requirement.

[mikehardy]

@ryanwilson - I've seen you helping out in the firebase-ios-sdk repo, thanks for stopping by here to clarify, I appreciate it.

Your expectation obviously makes sense but runs counter to the user report here.

I haven't replicated that but it seems like the next step is to make 100% sure - @dlockwo did you actually build with FirebaseCrashlytics and see a build failure, resulting in your removal of that module, or did you pre-emptively remove it after inspecting the podspec / seeing FirebaseCore and assuming it would pull in analytics?

[mikehardy]

@ryanwilson - thanks again (and thanks @wilhuff for the quick referral), that was the info I needed to get a handle on it.

The problem was on our side (as I'm sure you guessed)

We still had a rogue 'Firebase/Core' transitive dependency in our 'RNFBCrashlytics' podspec,

...even though

1. 'Firebase/CoreOnly' is sufficient and
2. there's already a dependency in RNFBCrashlytics.podspec to RNFBApp.podspec, which then transitively satisfies the 'Firebase/CoreOnly' need

I thought those direct 'Firebase/Core' dependencies were all cleaned out but that one stray remainder was the culprit

@dlockwo when I remove that 'Firebase/Core' dependency in RNFBCrashlytics.podspec I build and run successfully on iOS with Crashlytics, and without AdMob and Analytics, and the Podfile.lock confirms the FirebaseAnalytics and GoogleAppMeasurement pods are no longer pulled in

You may run it yourself as a test with my demonstrator using the new --no-idfa argument https://github.com/mikehardy/rnfbdemo/blob/master/make-demo.sh

The required patch to make it work, in patch-package format, is here https://github.com/mikehardy/rnfbdemo/blob/master/patches/%40react-native-firebase%2Bcrashlytics%2B8.3.3.patch

I'll have that integrated and released as @react-native-firebase/crashlytics v8.3.4 shortly

[dlockwo]

Cool. I was clearly looking at the wrong podspec in the IOS SDK. Thanks for sorting it. Would love to use Crashlytics in our kids app.