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(android): remove core requirement #136

Merged
merged 7 commits into from
Dec 28, 2021
Merged

feat(android): remove core requirement #136

merged 7 commits into from
Dec 28, 2021

Conversation

m1ga
Copy link
Collaborator

@m1ga m1ga commented Oct 29, 2021

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 😄

  • 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
Copy link
Owner

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

@m1ga
Copy link
Collaborator Author

m1ga commented Oct 29, 2021

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 😄

@jordanbisato
Copy link
Contributor

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

@m1ga
Copy link
Collaborator Author

m1ga commented Oct 29, 2021

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

@m1ga
Copy link
Collaborator Author

m1ga commented Oct 29, 2021

@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
Copy link
Contributor

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
Copy link
Collaborator Author

m1ga commented Oct 29, 2021

core is 135kb 😄 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
Copy link
Collaborator Author

m1ga commented Oct 30, 2021

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
Copy link
Collaborator Author

m1ga commented Nov 18, 2021

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
Copy link
Collaborator Author

m1ga commented Nov 22, 2021

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
Copy link
Collaborator Author

m1ga commented Dec 24, 2021

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

@hansemannn
Copy link
Owner

@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
Copy link
Collaborator Author

m1ga commented Dec 24, 2021

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
Copy link
Owner

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
Copy link
Contributor

jordanbisato commented Dec 25, 2021

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
Copy link
Collaborator Author

m1ga commented Dec 25, 2021

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

@hansemannn
Copy link
Owner

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
Copy link
Collaborator Author

m1ga commented Dec 25, 2021

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: tidev/ti.playservices#230 / tidev/ti.playservices#229

@hansemannn
Copy link
Owner

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?

@hansemannn hansemannn merged commit de83358 into main Dec 28, 2021
@m1ga
Copy link
Collaborator Author

m1ga commented Dec 28, 2021

@hansemannn done 👍 I'll check the other README files later

@m1ga m1ga deleted the coreUpdate branch February 13, 2022 20:04
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.

3 participants