feat(android): remove core requirement

m1ga commented 3 years ago

Remove firebase.core as a requirement since it will include AdMob (even if it is not used). According to Google: https://firebase.google.com/support/release-notes/android it is not needed anymore.

The Firebase Android library firebase-core is no longer needed. This SDK included the Firebase SDK for Google Analytics.
Raised the fcm version

Added methods for determining and controlling whether Google Play services is set as the app’s notification delegate. By default, FCM will now set Google Play services as the app’s notification delegate so that it is allowed to display notifications for the app. This could be used in the future to show an app’s notifications without needing to start the app, which may improve message reliability and timeliness.

Couldn't find any anything in the documentation but it still works so why not update it :smile:

added FLAG_IMMUTABLE to fix Targeting S+ (version 31 and above) requires that one of FLAG_IMMUTABLE or FLAG_MUTABLE be specified when creating a PendingIntent.

Tests I did test fcm, analytics, performance, crash, auth without core and they work. So we can remove the requirement from timodule.xml too

iOS ~~I didn't test iOS yet and don't know if it is still needed there.~~ Still has to be there. Added it back to the readme.

firebase.cloudmessaging-android-3.3.0.zip

hansemannn commented 3 years ago

So why would anyone use firebase-core now at all? Still don't get it..

m1ga commented 3 years ago

I guess only for Ti < 9 without gradle or in very special cases if you want to use a different configuration than provided in the json file. I don't know when that you don't need it message appeared but when I've searched for the admob detected message I found some react native issues where people have the same and they just need to remove the core part. It might have to do with the new data security regulations that Google isn't allowed to add AdMob in every app if you just use push :smile:

jordanbisato commented 3 years ago

@m1ga Did you test it with Analytics 19.0.0(ti.firebase-analytics 5.0.0) or it starts after 19.0.2?

m1ga commented 3 years ago

I've tested analytics-5.0.0, auth-3.1.0, performance-2.0.0, crashlytics-2.1.0

m1ga commented 3 years ago

@jordanbisato since you use Analytics it shouldn't make any difference since you have to set the tracking option in Google Play anyway. I don't use any tracking but I can't set it to "no tracking" because it finds the (not used) library in my dependencies.

jordanbisato commented 3 years ago

I was thinking about the size decrease of aab/apk and project. And maybe a improvement of performance. DO you think it's possible?

m1ga commented 3 years ago

core is 135kb :smile: do you see any performance issues when using the libraries? But I don't think it is part of this PR so please start an issue in the libraries repo if you can reproduce it with analytics

m1ga commented 3 years ago

Ok, so my tests look good! Android works fine without core. iOS still needs it otherwise it won't compile so I've put it back to the README. I removed some old texts and updated the FCM library

m1ga commented 3 years ago

Tests are fine. Ready to merge @hansemannn unless you've found something.

To see if AdMob is still in there you can do:

go into /build/android
run ./gradlew -q dependencies app:dependencies | grep -i adbmob
in the old version there should be a hit, without core there isn't (and push is still working)

m1ga commented 3 years ago

added FLAG_IMMUTABLE to fix Targeting S+ (version 31 and above) requires that one of FLAG_IMMUTABLE or FLAG_MUTABLE be specified when creating a PendingIntent.

m1ga commented 2 years ago

using this in a store app now, works fine and no "AdMob" is found :+1: @hansemannn: I'm ready to merge if its fine

hansemannn commented 2 years ago

@m1ga I am still a bit unsure because of the other modules. Is Firebase-Core never required anymore? This may be an important docs change.

m1ga commented 2 years ago

For iOS you'll still need it. I've changed the README so it lists firebase.core only for iOS as a requirement and put an if OS_IOS in the example. I use fcm and analytics in a released app without firebase.core (with ti 9+ it will just load the config file with gradle).

But you are right: we need to update the titanium-firebase README too.

edit: and it is not a problem if you still use it. It will just say you are using AdMob inside the "say what your app is using" section of the store. And you can't say you don't use it. If that isn't a problem for users (e.g. if they use ads in their app) it doesn't really matter since you have to say that you are using ads anyway.

hansemannn commented 2 years ago

Ok, so none of the Android modules (storage, auth, FCM, in-app-messing, config) need it, but the iOS ones do? That's pretty straight forward! I plan to merge the open iOS PR's today / tomorrow as well, then we have some fresh slices of modules ready!

jordanbisato commented 2 years ago

Yes, i'm using Config, Analytics, Performance, CloudMessaging, In-app Messaging and Crashlytics in a production android app without Core and it is working normally.

m1ga commented 2 years ago

Thanks for the feedback @jordanbisato :+1: That's good to know, haven't used all together in one production app yet.

hansemannn commented 2 years ago

Okay, then let's go! Should we maybe combine it with an update-to-latest for all libs? And I think the current play-services in Titanium are also outdated, not sure what to do about that.

m1ga commented 2 years ago

18.0.0/.1 was released this December: https://mvnrepository.com/artifact/com.google.android.gms/play-services-base?repo=google 17.6.0 was the last version before that. So it is not that old.

And we can update https://github.com/appcelerator-modules/ti.playservices/blob/master/android/build.gradle to 18.0.1

dependencies {
        // Needed to check for Google Play Services availability on device.
        implementation 'com.google.android.gms:play-services-base:18.0.1'

        // App devs expect adding "ti.playservices" will enable "Fused Location" support to "Ti.Gelocation" APIs.
        // So, we must add this library to support it, even though this module doesn't use this library at all.
        implementation 'com.google.android.gms:play-services-location:19.0.0'
}

builds fine but I didn't do any tests. Changelog of play-services: https://developers.google.com/android/guides/releases#december_09_2021 and 18.0.1: https://developers.google.com/android/guides/releases#december_16_2021

But I think we should move that to a PR in ti.playservices. FCM works fine with the current ti.playservice module so we can update this independently

issue/PR for ti.playservice: https://github.com/appcelerator-modules/ti.playservices/pull/230 / https://github.com/appcelerator-modules/ti.playservices/issues/229

hansemannn commented 2 years ago

Yep, let's merge this one first and the version upgrade separately. Can someone also remove it from the other Firebase Android modules? Merging this one already! @m1ga If you don't mind, can you release the new version?

m1ga commented 2 years ago

@hansemannn done :+1: I'll check the other README files later

hansemannn / titanium-firebase-cloud-messaging

feat(android): remove core requirement #136