search

xamarin / GoogleApisForiOSComponents

MIT License
225 stars 161 forks source link

[Update] Firebase for iOS v7.10.0 #479

Closed dmariogatto closed 3 years ago

dmariogatto commented 3 years ago

For review, @Redth @mattleibow @SotoiGhost @dalexsoto

Includes

Firebase

Google

Notes

Removed the Wrap methods/properties that where marked as Obsolete (seems like a major version bump is the time to do it 😅).

There were quite a few binding changes, so would definitely appreciate someone to look them over. All the samples do run & link as far as I can tell.

Also happy to separate the UMP package as was done in #459.

Any issues let me know!

mayhammf commented 3 years ago

Hi @dmariogatto, Apple is rejecting apps which include Firebase due to tracking transparency. I found a discussion on the firebase repo about this issue: https://github.com/firebase/firebase-ios-sdk/issues/7736

I’m wondering if your intention with PR was to update to the latest firebase SDK?

mayhammf commented 3 years ago

And apologies for spamming your PR @dmariogatto, but it is literally the only thing I could find on the internet that seems to be close enough to addressing the latest Firebase SDK update for Xamarin

mayhammf commented 3 years ago

Here is another reference about the rejection issues. https://github.com/firebase/quickstart-unity/issues/994

It seems like version 7.9.0-k2 is where the issue was resolved. Afterwards for version 7.11.0 they introduce a new Pod Firebase/AnalyticsWithoutAdIdSupport.

dmariogatto commented 3 years ago

@mayhammf I wasn't aware of firebase/firebase-ios-sdk#7736.

Looks like we'd have to separate out GoogleAppMeasurement into two different NuGets (one with tracking and one without). Then we'd have a matching pair of NuGets for Analytics.

I'm happy to do the work, but would like to get the okay from someone at Microsoft/Xamarin on the approach.

mayhammf commented 3 years ago

@dmariogatto i agree, there needs to be a package for AnalyticsWithoutAdIdSupport

However, the approach the firebase team took (introducing a different package) has the potential of leading to a chain reaction of dependency nightmares (at least in world of NuGet) where every package that might relay on Analytics has to (possibly) be duplicated (once with ad id support and one without)

As far as i can see on NuGet.org under Used by for Xamarin.Firebase.iOS.Analytics/, not a lot of packages are depending on Analytics, but it is still going to be complicated.

Do you see what i mean?

dmariogatto commented 3 years ago

@mayhammf I see your point about a dependency nightmare, but it might be unavoidable, unless Firebase changes the way they package Analytics.

mayhammf commented 3 years ago

@dmariogatto do you agree, now with v7.11.0 is out, that your PR should target that version instead of v7.10.0? I think that would bring us closer to the solution. Then a NuGet package can be introduced Firebase/AnalyticsWithoutAdIdSupport This way those who depend in Analytics and don't want to be stuck in review can get their apps deployed.

Do you think this is something @Redth and team will have to pre-approve first?

dmariogatto commented 3 years ago

@mayhammf Updating to 7.11.0 isn't that difficult, at a glance I can't see any binding changes, so mainly just updating the native frameworks. Would like to get some indication that the PR is going to be reviewed before spending the time.

mayhammf commented 3 years ago

@dmariogatto i agree with you. It makes a lot of sense to have a indication from Microsoft/Xamarin that they will be looking into this PR before spending the time.

Do you think introducing a new package for Firebase/AnalyticsWithoutAdIdSupport (7.11.0) is good idea? and would that be a big task?

We're contemplating branching out from your 710 branch and attempting to update to 7.11 and create a new project for AnalyticsWithoutAdIdSupport. But, to be honest, we are not sure that we know enough about this repo to pull this of.

hvaughan3 commented 3 years ago

Perhaps this is a dumb question, but why can't the native iOS firebase plugin not have a precompile directive (or similar swift concept) or a setting in info.plist to key off of in order to remove Ad ID usage? Or could we not remove the code by default and an extra library could be installed which would enable its use? I know the latter would be breaking change but this would prevent needing 2 separate libraries for everything.

dmariogatto commented 3 years ago

@hvaughan3 Not a dumb question! It might be possible to select which GoogleAppMeasurement framework is linked via a msbuild property.

For example in your project if you wanted Firebase without Ad Id support you'd have,

<PropertyGroup>
  <FirebaseWithoutAdIdSupport>True</FirebaseWithoutAdIdSupport>
</PropertyGroup>
dmariogatto commented 3 years ago

@mayhammf @hvaughan3

This is my quick attempt firebase-711.

The interesting file is Core.targets, which switches the linked GoogleAppMeasurement framework based on FirebaseWithoutAdIdSupport.

mayhammf commented 3 years ago

@dmariogatto @hvaughan3 loading the native framework conditionally is a very clever solution.! As long as firebase won't be introducing differences to the API (with and without AD support) i think this will be a perfect way to go about it.

GalaxiaGuy commented 3 years ago

Is there any chance these changes could include creating GoogleUserMessagingPlatform as a separate nuget package?

https://github.com/xamarin/GoogleApisForiOSComponents/issues/455 https://github.com/xamarin/GoogleApisForiOSComponents/pull/459

dmariogatto commented 3 years ago

@GalaxiaGuy #459 is mentioned in the PR description. Happy for it to be included if approved by a maintainer.

GalaxiaGuy commented 3 years ago

@dmariogatto 👍 Apologies, I missed that.

For some extra reading, a lot of people are unhappy with the Google UMP solution, and the Admob support are suggesting it will remain barebones and you can get a more featureful implementation elsewhere. So even people who use Admob might not need the UMP code: https://groups.google.com/g/google-admob-ads-sdk/c/JbiInGZ3sQ4/m/Z0VCbKnmAQAJ

dmariogatto commented 3 years ago

Now obsolete, see #485