bring back firebase, fix android push notifications#124
bring back firebase, fix android push notifications#124jpike88 wants to merge 18 commits intocapacitor-community:masterfrom
Conversation
|
These changes look good to me. Unfortunately, I'm unable to test Android. Are you cool with these @jcesarmobile ? |
This comment was marked as abuse.
This comment was marked as abuse.
|
The plugin never had firebase notifications implemented, it had a dependency to firebase messaging, but no code. Anyway, I don't have experience with intercom, so it's up to you what you merge and what not. |
|
why you can't use jcesar is right, we should not pin firebase messaging dependencies here. |
|
I do use I didn't just make this PR for fun, I made it because this repo does not work. My fix makes it work. Up to you guys but I have no reason to move back to main repo when it's literally broken for me. |
stewones
left a comment
stewones
left a comment
There was a problem hiding this comment.
We don't want to force external dependencies in plugins unless strictly necessary. Additionally, adding Firebase Messaging for Android only would break interoperability with iOS.
I hope my comments help you implement your app in a balanced way, where it fixes your issues while still using this plugin.
| androidTestImplementation "androidx.test.ext:junit:$androidxJunitVersion" | ||
| androidTestImplementation "androidx.test.espresso:espresso-core:$androidxEspressoCoreVersion" | ||
| implementation "io.intercom.android:intercom-sdk:$intercomSdkVersion" | ||
| implementation "com.google.firebase:firebase-messaging:$firebaseMessagingVersion" |
There was a problem hiding this comment.
@jpike88 have you tried defining the firebase-messaging dep in your app's build.gradle instead ?
| @@ -1,5 +1,12 @@ | |||
| <manifest xmlns:android="http://schemas.android.com/apk/res/android" | |||
| > | |||
| <application> | |||
There was a problem hiding this comment.
same here, you should be able to define this in your app layer, not in the plugin.
| @@ -0,0 +1,29 @@ | |||
| package com.getcapacitor.community.intercom.intercom; | |||
There was a problem hiding this comment.
this MessagingService could be implemented in the MainActivity if using the app's AndroidManifest
| String apiKey = config.getPluginConfiguration("Intercom").getString("androidApiKey"); | ||
| String appId = config.getPluginConfiguration("Intercom").getString("androidAppId"); | ||
|
|
||
| switch (IntercomPushManager.getInstalledModuleType()) { |
There was a problem hiding this comment.
not sure about this part as I can't test. but do we really need to tell Intercom to cache a sender id? if so, something to investigate and move to the app layer as well.
3 participants
fixes #87