Skip to content

feat(analytics, ATT): allow use of AnalyticsWithoutAdIdSupport pod #5224

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

Merged
merged 2 commits into from
Apr 29, 2021

Conversation

mikehardy
Copy link
Collaborator

@mikehardy mikehardy commented Apr 28, 2021

Description

Allow use of new Analytics subspec that gets you Analytics without the Ad Id usage (thus no need for Apple App Transparency Tracking)

Related issues

Discussion here #5223

Release Summary

2 conventional commits, can be rebase merged

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

  • test with firebase-ios-sdk 7.10.0 and using new podspec variable (should fail, does fail ✔️ )
  • test with firebase-ios-sdk 7.11.0, using new variable, with admob too (should fail, does fail ✔️ )
  • test with firebase-ios-sdk 7.11.0 using new variable, no admob (should use new subspec, works ✔️ )

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

@vercel
Copy link

vercel bot commented Apr 28, 2021

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/4qUcp1rrAAoWXromot4tPd5FzwrV
✅ Preview: https://react-native-f-git-mikehardy-ios-analytics-no-ads-in-9d841d.vercel.app

@mikehardy mikehardy marked this pull request as draft April 28, 2021 05:49
@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #5224 (01473f5) into master (99f8d2c) will decrease coverage by 52.30%.
The diff coverage is n/a.

❗ Current head 01473f5 differs from pull request most recent head ebc7f20. Consider uploading reports for the commit ebc7f20 to get more accurate results

@@             Coverage Diff             @@
##           master    #5224       +/-   ##
===========================================
- Coverage   88.86%   36.57%   -52.29%     
===========================================
  Files         109       51       -58     
  Lines        3743     1518     -2225     
  Branches      360      360               
===========================================
- Hits         3326      555     -2771     
- Misses        370      733      +363     
- Partials       47      230      +183     

@Salakar
Copy link
Contributor

Salakar commented Apr 28, 2021

Ruby code looks ok to me so not sure why it's not picking it up. Also I think it'd be worth having this mentioned in the analytics documentation somewhere

@mikehardy
Copy link
Collaborator Author

I will definitely put it in the docs - once it works :-)

@mikehardy mikehardy changed the title WIP feat(analytics, ATT): allow use of AnalyticsWithoutAdIdSupport pod feat(analytics, ATT): allow use of AnalyticsWithoutAdIdSupport pod Apr 29, 2021
@mikehardy mikehardy dismissed a stale review via 1eb3200 April 29, 2021 15:14
@mikehardy mikehardy force-pushed the @mikehardy/ios-analytics-no-ads branch from 242c7c2 to 1eb3200 Compare April 29, 2021 15:14
@mikehardy mikehardy marked this pull request as ready for review April 29, 2021 15:15
@mikehardy
Copy link
Collaborator Author

Tested and all appears to work today as I took this up again, so I think it's ready to go technically.
Added a nice documentation chunk on the analytics install that explains why you would want this,
how to do it, minimum firebase-ios-sdk requirement, AdMob incompatibility and everything

- not compatible with AdMob, so put a warning in there
- requires firebase-ios-sdk 7.11.0 but desire non-breaking, fail on install if req not met
- add messaging during install to make new option more developer discoverable
@mikehardy
Copy link
Collaborator Author

Last change was moving the entire documentation block to a top-level header (instead of inside the firebase.json block as a sub-head), everything else was green -> merging

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.

2 participants