invertase / react-native-firebase

🔥 A well-tested feature-rich modular Firebase implementation for React Native. Supports both iOS & Android platforms for all Firebase services.
https://rnfirebase.io
Other
11.63k stars 2.2k forks source link

🔥 [🐛] `FirebaseMessagingTypes.RemoteMessage` don't cover every possible remote message type on IOS #6491

Closed Jawkx closed 1 year ago

Jawkx commented 2 years ago

Issue

Title says it all, the FirebaseMessagingTypes.RemoteMessage type don't expect remote message with fcm_options payload in data. The current data type expect data object to have key value pairs of {string :string } data?: { [key: string]: string }; But if you send message to IOS with image attached, it will return data as below, which would make Notifee unable to display the notification

{
   "data":{
      "fcm_options":{
         "image": "IMAGE URL"
      },
   },
   "from":"359303784940",
   "messageId":"1661410431196314",
   "mutableContent":true,
   "notification":{
      "body":"BODY",
      "title":"TITLE"
   },
   "sentTime":"1661410431"
}

Project Files

Javascript

Click To Expand

#### `package.json`: ```json "@react-native-firebase/analytics": "^14.9.0", "@react-native-firebase/app": "^14.9.0", "@react-native-firebase/messaging": "^14.9.0", "@react-native-firebase/crashlytics": "^14.9.0", ``` #### `firebase.json` for react-native-firebase v6: ```json { "react-native": { "crashlytics_debug_enabled": false } } ```

iOS

Click To Expand

#### `ios/Podfile`: - [ ] I'm not using Pods - [x] I'm using Pods and my Podfile looks like: ```ruby # N/A ``` #### `AppDelegate.m`: ```objc // N/A ```


Android

Click To Expand

#### Have you converted to AndroidX? - [ ] my application is an AndroidX application? - [x] I am using `android/gradle.settings` `jetifier=true` for Android compatibility? - [ ] I am using the NPM package `jetifier` for react-native compatibility? #### `android/build.gradle`: ```groovy // N/A ``` #### `android/app/build.gradle`: ```groovy // N/A ``` #### `android/settings.gradle`: ```groovy // N/A ``` #### `MainApplication.java`: ```java // N/A ``` #### `AndroidManifest.xml`: ```xml ```


Environment

Click To Expand

**`react-native info` output:** ``` System: OS: macOS 12.4 CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz Memory: 62.78 MB / 16.00 GB Shell: 5.8.1 - /bin/zsh Binaries: Node: 18.7.0 - /usr/local/bin/node Yarn: 1.22.10 - /usr/local/bin/yarn npm: 8.15.0 - /usr/local/bin/npm Watchman: 2022.08.15.00 - /usr/local/bin/watchman Managers: CocoaPods: 1.11.3 - /usr/local/bin/pod SDKs: iOS SDK: Platforms: DriverKit 21.4, iOS 15.5, macOS 12.3, tvOS 15.4, watchOS 8.5 Android SDK: API Levels: 29, 30, 31 Build Tools: 28.0.3, 29.0.2, 29.0.3, 30.0.2, 30.0.3, 31.0.0 System Images: android-26 | Google APIs Intel x86 Atom, android-29 | Intel x86 Atom_64, android-29 | Google APIs Intel x86 Atom, android-30 | Google APIs Intel x86 Atom, android-30 | Google APIs Intel x86 Atom_64 Android NDK: Not Found IDEs: Android Studio: 4.2 AI-202.7660.26.42.7351085 Xcode: 13.4.1/13F100 - /usr/bin/xcodebuild Languages: Java: 11.0.12 - /usr/bin/javac npmPackages: @react-native-community/cli: Not Found react: 17.0.2 => 17.0.2 react-native: 0.67.4 => 0.67.4 react-native-macos: Not Found npmGlobalPackages: *react-native*: Not Found ``` - **Platform that you're experiencing the issue on**: - [x] iOS - [ ] Android - [ ] **iOS** but have not tested behavior on Android - [ ] **Android** but have not tested behavior on iOS - [ ] Both - **`react-native-firebase` version you're using that has this issue:** - `e.g. 5.4.3` - **`Firebase` module(s) you're using that has the issue:** - `e.g. Instance ID` - **Are you using `TypeScript`?** - `Y/N` & `VERSION`


mikehardy commented 2 years ago

Hi there!

The type is defined here and you can use the GitHub Web UI to propose a PR. What typing do you propose?

https://github.com/invertase/react-native-firebase/blob/630142ff49ca269d1b55d0898767228d0cf5f808/packages/messaging/lib/index.d.ts#L111

Jawkx commented 2 years ago

What do you think about adding a generic type? Something like FirebaseMessagingType.RemoteMessage<EXPECTED DATA INTERFACE>. Because I think a data type that is not { [key: string]: string }; is kind of an edge case and changing it would actually break alot of stuff (at least that's the case with my project)

mikehardy commented 2 years ago

Unfortunately this will collide with the firebase-js-sdk types that we try to mirrot, which are string for the value:

https://firebase.google.com/docs/reference/js/messaging_.messagepayload#properties

But I understand the issue. how about string | { [key: string]: string } to keep it pretty concrete, with a comment that we are specifically overriding firebase-js-sdk types by allowing a string:string object through?

Jawkx commented 2 years ago

But it is super weird tho, why is the fcm_options goes into the data field (I send the message using firebase console web UI). Sorry for being abit ignorant (I'm not really familiar with native code) can i know which part of the code is the part that the remote message "hook" into the react native side?(Correct me if there is no such thing and i totally don't know what i'm talking about lol)

mikehardy commented 2 years ago

Honestly, I don't know - this is probably something you could bring up with the firebase-js-sdk folks as a documentation issue on that URL I mentioned. They fix those pretty quickly. If you are sending these from their console and their types don't match it, that's not at the level of this package

Here's the code that turns the inbound JSON into a message I believe: https://github.com/invertase/react-native-firebase/blob/main/packages/messaging/ios/RNFBMessaging/RNFBMessagingSerializer.m

When I do a package-wide grep on messaging for fcm-options or fcm_options I get zero hits, so I think it's just the fall through case where we build the data object with whatever is left: https://github.com/invertase/react-native-firebase/blob/630142ff49ca269d1b55d0898767228d0cf5f808/packages/messaging/ios/RNFBMessaging/RNFBMessagingSerializer.m#L83-L87

This could be verified by adding an NSLog there and watching the log on a device that's plugged in as you send a message

Jawkx commented 2 years ago

Hello, added NSLog to log out the this is what i get from the UserInfo in RNFCMessagingSerializer This i what i got:

{
    aps =     {
        alert =         {
            body = "notification text";
            title = "notification title";
        };
        "mutable-content" = 1;
    };
    "fcm_options" =     {
        image = "https://lh3.googleusercontent.com/2hDpuTi-0AMKvoZJGd-yKWvK4tKdQr_kLIpB_qSeMau2TNGCNidAosMEvrEXFO9G6tmlFlPQplpwiqirgrIPWnCKMvElaYgI-HiVvXc=w600";
    };
    "gcm.message_id" = 1661604588217796;
    "gcm.n.e" = 1;
    "google.c.a.c_id" = 2320443206876893248;
    "google.c.a.c_l" = notification;
    "google.c.a.e" = 1;
    "google.c.a.ts" = 1661604588;
    "google.c.a.udt" = 0;
    "google.c.fid" = "eAju_cqFS0UsuEOUvmBJ_6";
    "google.c.sender.id" = 1026037303279;
    link = "https://www.google.com/";
}

As suspected, ya the problem is there is no skip key being built to ignore the fcm_options (and log it somewhere else). Is this the issue from firebase side or it's in the scope of this library. Is it better to log it inside another properties of remote message?

mikehardy commented 2 years ago

Okay, this is starting to make a little more sense. We're just not expecting those fcm_options there because they are specifically defined to be deeper in the payload hierarchy on Apple platforms, specifically inside the APNS Payload:

https://firebase.google.com/docs/cloud-messaging/android/send-image

https://rnfirebase.io/messaging/server-integration#send-messages-with-image

So I could be wrong but I don't think we have a need to change the type here, I think we need to figure out why we are seeing JSON emitted that contrary to docs?

Restarting from the beginning: how are you generating this JSON and is the goal to show images as in the rnfirebase.io link above?

Jawkx commented 2 years ago

I'm sending the message via the firebase console web UI. The web UI looks something like this:

Screenshot 2022-08-27 at 10 04 04 PM

Where the Notification image url will be passed into fcm_option which will be appended into remoteMessage.data

The actual goal is to display a notification even if the app is in foreground. The way I achieve this is to have a messaging().onMessage() handler which will receive an FirebaseMessagingTypes.RemoteMessage object display a notification using notifee.displayNotification() method.

I'm unable to access the remoteMessage.data.fcmOption.image method without typing bad typescript code. Because from typescript perspective the fcm_option is a string due to the way FirebaseMessagingTypes.RemoteMessage["data"] is typed.

Another issue is since typescript assume the data object is all {string, string} I passed the remoteMessage.data straight intonotifee.displayNotification.data and it cause error in Notifee side (it's easier to fix this tho, just add a type check and clean up the data)

github-actions[bot] commented 1 year ago

Hello 👋, to help manage issues we automatically close stale issues.

This issue has been automatically marked as stale because it has not had activity for quite some time.Has this issue been fixed, or does it still require attention?

This issue will be closed in 15 days if no further activity occurs.

Thank you for your contributions.