-
Notifications
You must be signed in to change notification settings - Fork 6k
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
[expo-notifications][Android] map Expo and Firebase notifications correctly #30615
[expo-notifications][Android] map Expo and Firebase notifications correctly #30615
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @douglowder and the rest of your teammates on Graphite |
The Pull Request introduced fingerprint changes against the base commit: e1ca69c Fingerprint diff[
{
"op": "changed",
"source": {
"type": "dir",
"filePath": "../../packages/expo-notifications/android",
"reasons": [
"expoAutolinkingAndroid"
],
"hash": "a12805410193866c27ccf23179a53f3f7902e174"
}
}
] Generated by PR labeler 🤖 |
0acbab4
to
cee2e3e
Compare
7b3920e
to
fd3d9f8
Compare
Manual testing with the test app looks good:
|
...cations/android/src/main/java/expo/modules/notifications/notifications/debug/DebugLogging.kt
Outdated
Show resolved
Hide resolved
...cations/android/src/main/java/expo/modules/notifications/notifications/debug/DebugLogging.kt
Outdated
Show resolved
Hide resolved
...cations/android/src/main/java/expo/modules/notifications/notifications/debug/DebugLogging.kt
Show resolved
Hide resolved
...cations/android/src/main/java/expo/modules/notifications/notifications/debug/DebugLogging.kt
Show resolved
Hide resolved
b57b5dd
to
2e0f92b
Compare
…rectly (#30615) Since #7280, some fields in Android notification content have been remapped. This is done in order to align the results seen in JS with the fields passed into the Expo push notification service when it sends notifications, and to align the API across the iOS and Android platforms. However, this mapping is also happening when notifications are sent directly from Firebase. This leads to unexpected results where the `data` part of the notification payload does not appear correctly in the JS notification structures. This PR fixes that by heuristically detecting whether a notification is sent from Expo or not, and performing the appropriate mapping in either case. - Modified code in `NotificationSerializer` to detect when the `body` attribute is a JSON string (indicating that the notification was sent from Expo), and remapping the resulting bundles as appropriate for both cases. - Modified `NotificationsEmitter` and `mapNotificationResponse` in the JS code to correctly unpack data strings in the case of Expo notifications. - Added extensive debug logging, wrapped by `BuildConfig.DEBUG` so the logging does not happen in release/production builds. - Both Firebase and Expo notifications should be mapped correctly in the test app - Notification responses should be mapped correctly in both cases when tapping on a notification while the app is in background or not running - CI should pass <!-- Please check the appropriate items below if they apply to your diff. This is required for changes to Expo modules. --> - [ ] Documentation is up to date to reflect these changes (eg: https://docs.expo.dev and README.md). - [ ] Conforms with the [Documentation Writing Style Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md) - [ ] This diff will work correctly for `npx expo prebuild` & EAS Build (eg: updated a module plugin).
# Why Applies requested changes from: `https://github.com/expo/expo/pull/30615#discussion_r1693426405`
Applies requested changes from: `https://github.com/expo/expo/pull/30615#discussion_r1693426405`
Bundle serializedContent = new Bundle(); | ||
serializedContent.putString("title", extras.getString("title")); | ||
String body = extras.getString("body"); | ||
String projectId = extras.getString("projectId"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, don't special case for EAS Notify. Instead, special case for "JSON-like bodies".
Why
Since #7280, some fields in Android notification content have been remapped. This is done in order to align the results seen in JS with the fields passed into the Expo push notification service when it sends notifications, and to align the API across the iOS and Android platforms.
However, this mapping is also happening when notifications are sent directly from Firebase. This leads to unexpected results where the
data
part of the notification payload does not appear correctly in the JS notification structures.This PR fixes that by heuristically detecting whether a notification is sent from Expo or not, and performing the appropriate mapping in either case.
How
NotificationSerializer
to detect when thebody
attribute is a JSON string (indicating that the notification was sent from Expo), and remapping the resulting bundles as appropriate for both cases.NotificationsEmitter
andmapNotificationResponse
in the JS code to correctly unpack data strings in the case of Expo notifications.BuildConfig.DEBUG
so the logging does not happen in release/production builds.Test Plan
Checklist
npx expo prebuild
& EAS Build (eg: updated a module plugin).