Skip to content
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(ios): podspecs now utilize CoreOnly instead of Core #3575

Merged
merged 5 commits into from
May 5, 2020
Merged

feat(ios): podspecs now utilize CoreOnly instead of Core #3575

merged 5 commits into from
May 5, 2020

Conversation

dlockwo
Copy link
Contributor

@dlockwo dlockwo commented Apr 29, 2020

…xcept Analytics which should continue to reference Core.

Description

iOS does not allow applications in the Kids category to collect Firebase analytics. Firebase has refactored their libraries to utilize the CoreOnly module instead of Core to accommodate this. Currently RNFirebase pods rely on Firebase/Core. Pull request updates RNFirebase pod files to utilize CoreOnly instead of Core (except Analytics)

Related issues

Fixes #3570

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 e2e tests without errors. Tested libraries I use locally in my application (app, auth, database).

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

…xcept Analytics which should continue to reference Core.
@CLAassistant
Copy link

CLAassistant commented Apr 29, 2020

CLA assistant check
All committers have signed the CLA.

mikehardy
mikehardy previously approved these changes Apr 29, 2020
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems sensible enough based on my reading, @Ehesp I think you were looking at this over in flutter-land? You may have insight

@Ehesp
Copy link
Member

Ehesp commented Apr 29, 2020

Yep this is one we need to sort on FlutterFire too - thanks for submitting the PR. Looks like everything is Green too which is handy.

@Ehesp
Copy link
Member

Ehesp commented Apr 29, 2020

Infact, looking at the Podspec for Firebase, the pods already include CoreOnly as a dependency - just not versioned. cc @Salakar any ideas whether we even need to include these?

@dlockwo
Copy link
Contributor Author

dlockwo commented Apr 29, 2020

I wondered if they needed to be included as well, but followed the convention in place..

@Salakar
Copy link
Member

Salakar commented Apr 29, 2020

They no longer need to be added it seems, so they can be safely removed - would you mind making the change to do that @dlockwo?

@dlockwo
Copy link
Contributor Author

dlockwo commented Apr 30, 2020

I've made the changes, but wanted your thoughts on the following pods: AdMob, DynamicLinks, InAppMessaging and RemoteConfig.

The Firebase documentation for DynamicLinks, InAppMessaging and RemoteConfig specify you should add that pod and Analytics. The Firebase podspecs for these libraries pull in the FirebaseAnalyticsInterop pod which I assume interfaces with Firebase/Analytics if it exists. E2E tests pass without Analytics included.

The original RNFirebase AdMob podspec included Analytics. The documentation for AdMob does not reference Firebase/AdMob at all. It instead references the pod Google-Mobile-Ads-SDK. E2E tests pass without Analytics included.

Do you want to include Analytics in these podspecs or instruct users to add the Analytics library separately?

@Salakar
Copy link
Member

Salakar commented May 5, 2020

Thanks for making these changes;

The Firebase documentation for DynamicLinks, InAppMessaging and RemoteConfig specify you should add that pod and Analytics. The Firebase podspecs for these libraries pull in the FirebaseAnalyticsInterop pod which I assume interfaces with Firebase/Analytics if it exists. E2E tests pass without Analytics included.

If this is what the docs suggest then I think we should have the Analytics pod by default in each of these.

The original RNFirebase AdMob podspec included Analytics. The documentation for AdMob does not reference Firebase/AdMob at all. It instead references the pod Google-Mobile-Ads-SDK. E2E tests pass without Analytics included.

Firebase/AdMob is just a shell podspec afaik that brings in the Google-Mobile-Ads-SDK pod automatically - It's easier using this one as when we update the Firebase iOS SDK version then this also covers updating the Ads SDK

Copy link
Member

@Salakar Salakar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to go ahead and merge this - will sort the analytics pods separately (will conditionally add them in if RNFB analytics package is installed in users project). Messaging is another one: https://firebase.google.com/docs/cloud-messaging/ios/client

Thanks for taking the time to see this PR through 🎉

@Salakar Salakar changed the title fix(ios): Changed podspecs to utilize CoreOnly instead of Core. All e… feat(ios): podspecs now utilize CoreOnly instead of Core May 5, 2020
@Salakar Salakar merged commit 35285f1 into invertase:master May 5, 2020
Salakar added a commit that referenced this pull request May 8, 2020
stefkampen pushed a commit to stefkampen/react-native-firebase that referenced this pull request Jun 6, 2020
stefkampen pushed a commit to stefkampen/react-native-firebase that referenced this pull request Jun 6, 2020
andersondanilo pushed a commit to vixtech/react-native-firebase that referenced this pull request Nov 9, 2020
andersondanilo pushed a commit to vixtech/react-native-firebase that referenced this pull request Nov 9, 2020
hmhm2292 pushed a commit to hmhm2292/react-native-firebase that referenced this pull request Jul 13, 2021
hmhm2292 pushed a commit to hmhm2292/react-native-firebase that referenced this pull request Jul 13, 2021
androidIsForVivek pushed a commit to androidIsForVivek/react-native-firebase that referenced this pull request Aug 9, 2021
androidIsForVivek pushed a commit to androidIsForVivek/react-native-firebase that referenced this pull request Sep 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

🔥Replace Firebase/Core dependency with Firebase/CoreOnly on iOS
5 participants