Removed Public Dependency on Firebase Messaging#28
Removed Public Dependency on Firebase Messaging#28
Conversation
| open fun onPushNotificationClicked(pushNotification: Map<String, String>) {} | ||
|
|
||
| open fun onPushNotificationDelivered(message: RemoteMessage) {} | ||
| open fun onPushNotificationDelivered(pushNotification: Map<String, String>) {} |
There was a problem hiding this comment.
We can no longer expose the public class at this point. Using a simple map is more ideal.
There was a problem hiding this comment.
Thinking: since we're now taking an implementation dependency on Firebase Messaging, we could reference RemoteMessage in the Courier SDK, however we avoid referencing Firebase classes in Courier's public API since they're not exposed through the Courier SDK
| import com.google.firebase.messaging.RemoteMessage | ||
|
|
||
| fun RemoteMessage.presentNotification(context: Context, handlingClass: Class<*>?, icon: Int = android.R.drawable.ic_dialog_info, settingsTitle: String = "Notification settings") { | ||
| fun CourierPushNotificationIntent.presentNotification(title: String?, body: String?, icon: Int = android.R.drawable.ic_dialog_info, settingsTitle: String = "Notification settings") { |
There was a problem hiding this comment.
Moved this extension to the intent class
| } | ||
| } | ||
|
|
||
| val RemoteMessage.pushNotification: Map<String, Any?> |
There was a problem hiding this comment.
Dropping this for now. This extension is used by react native and flutter. We will likely need to massage things a bit more when we get there.
There was a problem hiding this comment.
Does that mean those SDKs will need updates, too?
| } | ||
|
|
||
| // Returns the last message that was delivered via the event bus | ||
| fun onPushNotificationEvent(onEvent: (event: CourierPushNotificationEvent) -> Unit) = coroutineScope.launch(Dispatchers.Main) { |
There was a problem hiding this comment.
We may want to change this to use a listener of some sort in the future. Not a huge deal for now
danasilver
left a comment
danasilver
left a comment
There was a problem hiding this comment.
Thinking: this should be a major version bump of the SDK since it's a breaking change, and we ought to document which versions of the Firebase BOM/Firebase Messaging SDK are supported
|
|
||
| // Firebase | ||
| implementation platform('com.google.firebase:firebase-bom:XXXX') | ||
| implementation "com.google.firebase:firebase-messaging" |
There was a problem hiding this comment.
nit: These could use consistent syntax:
implementation platform("com.google.firebase:firebase-bom:X.Y.Z")
implementation("com.google.firebase:firebase-messaging")The Android docs use double quotes here, too, if we want to stay consistent with those examples: https://developer.android.com/develop/ui/compose/bom#kts
| open fun onPushNotificationClicked(pushNotification: Map<String, String>) {} | ||
|
|
||
| open fun onPushNotificationDelivered(message: RemoteMessage) {} | ||
| open fun onPushNotificationDelivered(pushNotification: Map<String, String>) {} |
There was a problem hiding this comment.
Thinking: since we're now taking an implementation dependency on Firebase Messaging, we could reference RemoteMessage in the Courier SDK, however we avoid referencing Firebase classes in Courier's public API since they're not exposed through the Courier SDK
| @@ -1,4 +1,4 @@ | |||
| <?xml version="1.0" encoding="utf-8"?> | |||
| <manifest xmlns:android="http://schemas.android.com/apk/res/android"> | |||
| } | ||
| } | ||
|
|
||
| val RemoteMessage.pushNotification: Map<String, Any?> |
There was a problem hiding this comment.
Does that mean those SDKs will need updates, too?
| data class CourierPushNotificationEvent( | ||
| val trackingEvent: CourierTrackingEvent, | ||
| val remoteMessage: RemoteMessage | ||
| val data: Map<String, String> |
There was a problem hiding this comment.
I'd consider creating a class for the message data (ex. CourierMessage or something) that's more descriptive to pass around than a plain Map and let's us extend functionality down the line
2 participants
Now, the firebase dependency does not get exposed publicly. This will allow developers to use different versions of firebase messaging in their projects than what we have in the package.
This change makes things a bit more flexible than what we currently offer, but does add a couple more steps to get up and running with Courier.