TobiasBuchholz / Plugin.Firebase

Wrapper around the native Android and iOS Firebase Xamarin SDKs
MIT License
220 stars 49 forks source link

Feature/android sdk updates #299

Closed vhugogarcia closed 1 week ago

vhugogarcia commented 5 months ago

Hello @TobiasBuchholz ,

I went ahead and updated the Android SDKs versions, and documentation as well. I updated to a newer version of the Android SDK, which is not the latest but it is the currently the stable ones and good in preparation for the future.

Also, I updated to .NET 8. Please give me a hand reviewing it, and if approve it, please feel free to edit/merge it. If you want me to update other bits, please let me know.

I believe this can be part of the new version 3.0.0, so the NuGet packages are all updated for both platforms.

Regards, Victor

AdamEssenmacher commented 5 months ago

Updating those target conditions to be version-agnostic is a great idea!

I'm not so sure about the Android version bumps though. I'll explain.

With NuGet package versioning, when we set a dependency version like

<PackageReference Include="Xamarin.AndroidX.Core" Version="1.12.0.2" />

we're actually declaring the minimum package version needed.

When publishing an application, it's often good practice to set these minimums to latest dependency versions so that we get all the latest features and bug fixes. Change is risk though, and sometimes updates break things. This is why when publishing a library like this plugin, these minimums should be set to the actual minimum the library needs. This approach avoids unnecessary version conflicts and maximizes compatibility with other libraries and projects.

Some reasons we'd want to bump minimum dependency versions for this plugin:

Unless these version bumps fall into one of these categories (or maybe one I didn't think of), I don't think it makes sense to change them right now.

Maybe we could instead update the plugin README to include a recommended set of <PackageReference />s to make it clear to consumers that they should be managing these dependencies.

AdamEssenmacher commented 5 months ago

The same goes for .net8. We should have a reason for excluding consumers that might be stuck on .net7 or (god help them) .net6.

vhugogarcia commented 5 months ago

All those are good points about the Android SDK, however, if you see the table version comparison below the updates to the packages are from minimal to major updates that solve some issues and add new features:

SDK Name From Binding Version (Firebase SDK Version) To Binding Version (Firebase SDK Version) Comments
Analytics 121.3.0.3 (21.3.0) 121.3.0.5 (21.3.0) Binding internal minor update
Auth 122.2.0 (22.2.0) 122.3.0.1 (22.3.0) Provides a few important updates and bug fixes outlined here.
Cloud Messaging 123.0.8 (23.0.8) 123.3.1.2 (23.3.1) This is a critical update, on June 21, 2024 the FCM legacy APIs will stop working, so it is important to update to version 23.3.1, so FCM keeps working. You can find more information here and here.
Crashlytics 118.3.2 (18.3.2) 118.6.0.1 (18.6.0) General improvements for reporting crashes on applications
DynamicLinks 121.0.2 (21.0.2) 121.2.0.2 (21.2.0) Minor update
Firestore 124.8.1.1 (24.8.1) 124.9.1.2 (24.9.1) Added support for sum and average aggregate queries. This is very helpful because we can later on update the plugin to support this new features which I'm sure, a lot of developers will see benefits.
Functions 120.3.1.3 (20.3.1) 120.4.0.2 (20.4.0) Minor update
RemoteConfig 121.1.2 (21.2.2) 121.6.0.1 (21.6.0) Added support for other firebase services to integrate with this service.
Storage 120.2.1.3 (20.2.1) 120.3.0.2 (20.3.0) General bug fixes

Now, speaking for myself, I personally see a lot of benefits on the Firestore one, because the aggregate queries help a lot to prevent firebase usage to go up, and help us to reduce costs (which we can contribute with these new features once the new version is released), and of course the push notifications to keep them working on Android, now that Android 15 final release is near.

About .NET 8, .NET 6 as you said Adam, I'm sure nobody is on .NET MAUI 6 and if yes, they can always use the previous version of this Plugin. For .NET 7, it is deprecated and unsupported, so same as before, they can continue using the previous version. However, .NET 8 is the long-term support and most stable version for .NET MAUI, the support will end on November, 2026, so I think this is the best version we can target for this plugin.

what do you think @AdamEssenmacher @TobiasBuchholz ?

AdamEssenmacher commented 5 months ago

Regarding .NET 8--the application determines the runtime version. Assemblies built using .net6/7 are backwards compatible. Nothing is lost by targeting 6. Newtonsoft.Json,Polly, and AutoMapper are examples of other library nugets that behave this way. If anything, we should add net8.0 to TargetFrameworks--though I'm not sure if that has any practical impact.

Regarding the android sdks, I'll reiterate that dependencies declared in these plugin packages are explicit minimums. Plugin consumers are free to pin later versions if they want. If we set minimums higher than necessary, then we take options away from developers for no tangible benefit. Our default position should be to not make these versioning decisions for them until it's absolutely necessary.

The only update I'm seeing here that meets this threshold is Cloud Messaging, as the deprecated FCM API renders prior versions effectively useless.

Adding support for Firestore aggregate / sum queries would be awesome, but until that's actually implemented, there's no need for the minimum version bump.

TobiasBuchholz commented 5 months ago

I share the same perspective as Adam in keeping the Android SDK dependency versions as low as possible to maximize compatibility with other libraries and projects. As long as the Plugin.Firebase codebase doesn’t implement any of the features offered by newer Android packages, it doesn’t make sense to simply bump up the versions. As Adam has already mentioned, adding support for Firestore aggregate/sum queries would be awesome, but until that is actually implemented, there’s no need for a minimum version bump.

Additionally, as both of you have already mentioned, since it is critical to keep FCM working, we should definitely update the Cloud Messaging package.

Regarding .NET 8, I also don’t see any benefit from updating for the same reasons Adam has already stated. However, if adding .NET 8.0 to TargetFrameworks has indeed a practical impact, we can certainly consider doing so.

vhugogarcia commented 5 months ago

Sounds good! However, one thing that is important to mention is that if we update the FCM to any new version the Android SDKs will also be required to be bumped up a little bit just like I did it on this PR. So, I still believe it does not hurt to upgrade the versions from all packages on this PR.

btw, Adding support for Firestore aggregate/sum/count queries, it is something I have on my plate for next month, so I can collaborate by adding the support for those on the Bindings for iOS first, and then add it here. 🙏🏻

I still believe we should migrate to .NET 8 on version 3.0.0, because for older versions users can still use the version 2.x.x of the plugin.

So, what should we do then as next step? hehehe 😃

AdamEssenmacher commented 5 months ago

For next steps, this branch/PR should be split into its discrete concerns:

1) One PR to focus on the minimum version bump(s?) needed to get us past the FCM issue (which I think is just Xamarin.Firebase.Messaging 1.23.2.1)

if we update the FCM to any new version the Android SDKs will also be required to be bumped up a little bit just like I did it on this PR.

I don't see why this would be necessary? The other features/components are unrelated.

2) The latest Xamarin.Firebase.* packages (released just a couple of weeks ago) drop support for .net6 and move to .net7, so that is on the horizon. It would be nice to have a branch upgrading this project to .net7 ready to go (for when we need it).