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

fix(messaging): RemoteMessage.data may be JSON-serializable object as well as string #7316

Merged

Conversation

ccorneliusHsv
Copy link
Contributor

Description

Discovered through testing that the type for data on the RemoteMessage could be a string or an object. This was

Related issues

Fixes #7279

Release Summary

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
    • [ X ] Yes
  • My change supports the following platforms;
    • [ X ] Android
    • [ X ] iOS
  • My change includes tests;
    • [ X ] e2e tests added or updated in packages/\*\*/e2e
    • jest tests added or updated in packages/\*\*/__tests__
  • [ X ] I have updated TypeScript types that are affected by my change.
  • This is a breaking change;
    • Yes
    • [ X] No

Test Plan

Updated e2e test to include an object and string to reflect updated types option.

Think react-native-firebase is great? Please consider supporting the project with any of the below:

@CLAassistant
Copy link

CLAassistant commented Aug 29, 2023

CLA assistant check
All committers have signed the CLA.

@vercel
Copy link

vercel bot commented Aug 29, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
react-native-firebase ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 11, 2023 0:39am
react-native-firebase-next ❌ Failed (Inspect) Sep 11, 2023 0:39am

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks correct, thank you for posting this!

@mikehardy mikehardy added the Workflow: Pending Merge Waiting on CI or similar label Sep 3, 2023
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And that's why we have tests 😆 thank you for adding the test change so it was covered or we may not have seen this

1) remoteMessage modular
       firebase v8 compatibility
         messaging().sendMessage(*)
           data
             accepts custom data value:
     NativeFirebaseError: [messaging/unknown] Value for fooObject cannot be cast from ReadableNativeMap to String
      at FirebaseMessagingModule.sendMessage (http://localhost:8081/index.bundle//&platform=android&dev=true&minify=false&app=com.invertase.testing&modulesOnly=false&runModule=true&sourceMapURL=true:25[2691](https://github.com/invertase/react-native-firebase/actions/runs/6016676986/job/16457624689?pr=7316#step:19:2695):28)
      at Context.<anonymous> (/Users/runner/work/react-native-firebase/react-native-firebase/packages/messaging/e2e/remoteMessage.e2e.js:182:40)

@mikehardy
Copy link
Collaborator

mikehardy commented Sep 3, 2023

Interesting - so, is it possible for it to truly be type string | object or is it more specific to say it may be string | <json serializable object> ? (that is, non-recursive etc)

Because the Java API for RemoteMessage.addData only accepts string/string parameters, implying that objects were not really contemplated as parameters: https://firebase.google.com/docs/reference/android/com/google/firebase/messaging/RemoteMessage.Builder#addData(java.lang.String,java.lang.String)

It's possible we can serialize the object to JSON prior to sending it down to native, and I think that's likely the best course - in lib/index.js checking if it is typof string (or similar?) and if it is not then doing a JSON.serialize(param, null, 0) on it before sending it down to the native layer.

What do you think?

@mikehardy mikehardy added Workflow: Waiting for User Response Blocked waiting for user response. and removed Workflow: Pending Merge Waiting on CI or similar labels Sep 3, 2023
Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

underlying API is string/string, so object must be serialized I think, which is easiest at javascript layer prior to sending it to native layer

@ccorneliusHsv
Copy link
Contributor Author

@mikehardy I'm working on the solution you suggested, finishing up tests and validations to give me some confidence it's gonna work correctly. I do want to provide some info and to make sure that this PR suggestion is valid. We are using the Firebase console to send notifications and the following is what we are receiving .

iOS => Push Notification - onMessage
{
"data":
{
"deep_link": "******",
"marketing": "true",
"fcm_options": {"image": "https://.png"}
},
"from": "
",
"messageId": "1694100993860917",
"mutableContent": true,
"notification": {"body": "Body Example", "title": "Title Example"},
"sentTime": "1694100993"
}

Android => Push Notification - onMessage
{
"collapseKey": "",
"data":
{
"deep_link": "",
"marketing": "true"
},
"from": "
",
"messageId": "0:1694103182522315%980f3695980f3695",
"notification":
{
"android":
{
"channelId": "marketing",
"imageUrl": "https://
**.png"
},
"body": "Body Example",
"title": "Title Example"
},
"sentTime": 1694103182513, "ttl": 2419200
}

So, we only see the type issue on the iOS notifications and we only see this issue when using the Firebase console to send notifications. Went through some sanity checks including looking at our Notifications Service to make sure we weren't modifying the notifications...and I don't see anything. Just wanted to give an update and the additional info in case it might be helpful.

Copy link
Collaborator

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a subtle one but a valid one - I think this matches up with underlying SDK params now as well as actual experience of getting objects in RemoteMessage 🤞

@mikehardy mikehardy added Workflow: Pending Merge Waiting on CI or similar and removed Workflow: Waiting for User Response Blocked waiting for user response. labels Sep 20, 2023
@mikehardy mikehardy changed the title fix: update RemoteMessage data type to be string | object fix(messaging): RemoteMessage.data may be JSON-serializable object as well as string Sep 20, 2023
@mikehardy mikehardy merged commit 7945a24 into invertase:main Sep 20, 2023
10 of 11 checks passed
@mikehardy mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Sep 20, 2023
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.

🔥 Type Issue in Messaging - Not a bug just a type fix
3 participants