-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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 FCM V1 Migration Tracking Issue #28656
Comments
Something else I noticed that might be worth noting is that notification images (not icons) don't seem to be working when the app is in the foreground. Edit: Just to clarify, I tested this by sending the request to the FCM V1 api directly |
To add on to "the notification icon is not working", on Android I'm finding that the icon displays when the app is in the foreground. But I get a blank circle as an icon when the app is in the background This is happening with a development build |
No issue with the icon when we add the correct name/value pair on the Android manifest but it seems there is no "plugin" on the package expo-notification or this one is not working as expected. But this issue can be fixed "simply" by adding a manual plugin specified here #24844 (comment) in addition to the |
Hi, I am using Expo SDK 49. |
For me, the plugin here is unnecessary because the AndroidManifest that's created seems fine. I have 2 notification_icon additions. |
I have the same issue on IOS. |
PR #28883 has been submitted to fix the Android notification response issues when app is in background or not running. |
Awesome. thank you so much for tracking this down for all of us. Any idea when it will be available? |
Second that! Really need it :) |
0.28.2 didn't fix anything :( |
In my case I can confirm that with the new version, |
@fdelu see the fix that was just added in #28883 , specifically the new method in That is the method that takes the bundle passed into If you are missing part of the payload, you should have a look to see if there are parts of the bundle that are not being mapped by |
@fdelu in looking at this further, it may be that you rebuilt your Android app with the new expo-notifications package, but did not rebuild your JS bundle to pick up the required changes on the JS side: https://github.com/expo/expo/pull/28883/files#diff-37b24aeb2e0d5d7c4365c987c51f03db4b6d20a0927f3eefe0d9e0a35b2c4df7 The |
Yes, that seems like it. I noticed later that I had "dataString" but not "data" so I'm probably missing that. Not sure how that happened though. Thanks for your help! |
After updating to expo-notifications@0.28.2 and building, this works 🎉. Thanks, @douglowder. Great work! 👏 ... the only minor issue is that the notification icon is not displaying correctly with FCM V1. |
An update on this @douglowder: I verified and it is using the latest version. When my app is in the foreground, I get a notificationd and click on it, I see the "response received" log and everything works fine. However, when my app is closed (fully), I get a notification and click on it to open the app, then I don't see that "response received" log, the "data" field is missing and the request content contains the "dataString" field. Looking at the code for Another issue I've been having is that if I get a notification when the app is open, I fully close it and then I reopen the app by clicking the notification, const notification = useLastNotificationResponse();
console.log("LAST RESPONSE", notification);
console.log(
"LAST RESPONSE CONTENT",
notification?.notification?.request?.content
);
console.log(
"LAST RESPONSE TRIGGER",
notification?.notification?.request?.trigger
); The logs:
|
@fdelu yes indeed that is a bug! I think cutting and pasting the new code from NotificationEmitter should fix it... I really appreciate you taking the time to track that down. |
Hi folks, if you're in this thread due to icon-related issues (@deivijt @zandvakiliramin @haplionheart): Can you try using a config plugin like in this comment to ensure the correct fields are in your manifest? And let me know if that changes anything? @krazykriskomar re: your comment here, I wasn't sure if you have it working or not? Can you confirm if icon behavior is correct for you when sending notifs using FCM V1? |
@appsgenie you should definitely move to SDK 51. We do plan to backport the FCMv1 fixes to SDK 50, but not 49. |
got it. thank you. and the existing clients will just work like it does now with |
Hi! @douglowder, will let @ctwillie (developer of one of the PHP SDKs available here https://github.com/ctwillie/expo-server-sdk-php) know that it might be some changes in the payload to expo notifications API. I also point that the expo tool and the expo notifications API differs in the API endpoint. Docs: https://exp.host/--/api/v2/push/send Expo tool: https://api.expo.dev/v2/push/send And if you can, please let @christopherwalter know that is needed to clarify the use of contentAvailable undocumented payload param for IOS, that at least is a little bit confusing, because some developers say you must send |
@nakedgun can you please tell me if you can capture the payload sent through node SDK to the notifications API, so I can compare it with mine and the PHP SDK, thanks a lot! |
@nakedgun just created a test project to use the node SDK
The code for implementing the node SDK is the very same that available in the project readme like this:
|
@douglowder Would you be able to confirm if the fixes are expected to be on SDK50 before the deadline at this point? Since we also have to account for a little bit of app store review time, I'm trying to figure out the safest option to ensure android notifications can keep working (upgrade to 51 vs wait for backport).
Thanks for your continued effort in investigating this issue. |
@jludwiggreenaction I think SDK 51 is in production at the moment |
Oh yea you're right. Guess I'm living under a rock. Rest of my question still stands though 🤦 |
@appsgenie our plan is to do what we can. :) Basically our current production app is on Expo v49 and we're currently sending out alerts via the "legacy" API ( So our plan is to fast-track a new v51 build so users can update before we are forced to move to Our situation does make it a little easier also, as our app is basically a business-line alerting app that's coupled to our main server products (facial rec/LPR, id verification, loss prevention, etc). So most of our users are security teams, which we can notify and will likely update in bulk during a morning meeting, etc -- assuming we can get it in the store quickly of course. 😅 |
@mgscreativa the nodejs code looks fine - I'm afraid I don't have time at the moment for capturing requests (it's not super trivial for https connections coming from docker containers). I would try adding the onNotificationResponseFromExtras patch and see if it fixes it - then report back. I would also check you're receiving other types of notifications and there are no errors when processing the push notification tickets. If you do hit issues, the best way to troubleshoot them is to attach a debugger to a physical device in Android Studio and start adding breakpoints, providing you're confident there's no issues with the server component sending out the notifications. @appsgenie If it helps, our rollout strategy is similar to @Codelica . Existing clients are on SDK49 with That said, come June 19th, like @Codelica , we will force this as an update for Android users (we also have a custom update mechanism which enforces minimum versions for iOS/Android). That way, come June 20th, all Android users will be on the build which supports the |
@nakedgun will crate a patch for the function onNotificationResponseFromExtras only |
Hi all, I've updated this issue description with the latest status on issues tracked throughout these comments. Here's the current status as of expo@51.0.11 and expo-notifications@0.28.8, please upgrade your packages if you haven't done so already. Let me know if there are any discrepancies :)
|
… app in background or killed (#29659) # Why The logic in `onNotificationResponseFromExtras` needs to be adjusted to correctly check whether response listeners are active, and do that check first before falling back to add the response to the pending responses (eventually used by `useLastNotificationResponse()` in JS). See #28656 (comment) # How Inverted the order of checks in the method. # Test Plan - CI should pass - Test the flows with the test app, using both expo-server-sdk tool and expo.dev/notifications web tool # Checklist <!-- 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).
@christopherwalter notifications@0.28.8 fails to handle notifications in first background of the app, but as stated by @nakedgun and tested by me this PR fixes it #29659 EDIT: patch to be applied in expo-notificatoins@0.28.8
|
Thank you so much for sharing your thoughts @Codelica and @nakedgun 🙏 Is the latest version of @Codelica you mentioned OTA update, but with this kinda change (new SDKs + packages) that won't be an option right? @nakedgun, what happens to old builds after June 20th? Like is it correct to say that starting June 20th, the notifications will act the same way as if I would do |
@appsgenie come June 20th (when the legacy API is depreciated), yes, it's effectively the same as In terms of diff --git a/node_modules/expo-notifications/android/src/main/java/expo/modules/notifications/notifications/NotificationManager.java b/node_modules/expo-notifications/android/src/main/java/expo/modules/notifications/notifications/NotificationManager.java
index 384a78b..172bc91 100644
--- a/node_modules/expo-notifications/android/src/main/java/expo/modules/notifications/notifications/NotificationManager.java
+++ b/node_modules/expo-notifications/android/src/main/java/expo/modules/notifications/notifications/NotificationManager.java
@@ -127,15 +127,26 @@
}
public void onNotificationResponseFromExtras(Bundle extras) {
- if (mPendingNotificationResponsesFromExtras.isEmpty()) {
- mPendingNotificationResponsesFromExtras.add(extras);
- } else {
+ // We're going to be passed in extras from either
+ // a killed state (ExpoNotificationLifecycleListener::onCreate)
+ // OR a background state (ExpoNotificationLifecycleListener::onNewIntent)
+
+ // If we've just come from a background state, we'll have listeners set up
+ // pass on the notification to them
+ if (!mListenerReferenceMap.isEmpty()) {
for (WeakReference<NotificationListener> listenerReference : mListenerReferenceMap.values()) {
NotificationListener listener = listenerReference.get();
if (listener != null) {
listener.onNotificationResponseIntentReceived(extras);
}
}
+ } else {
+ // Otherwise, the app has been launched from a killed state, and our listeners
+ // haven't yet been setup. We'll add this to a list of pending notifications
+ // for them to process once they've been initialized.
+ if (mPendingNotificationResponsesFromExtras.isEmpty()) {
+ mPendingNotificationResponsesFromExtras.add(extras);
+ }
}
}
} Also note case: #29622. |
@appsgenie, @nakedgun's post covers things well. But once Expo's push API must use the new API next week it's not that older Android builds won't receive notifications (go silent) -- technically they will go out via the new API and be delivered. However, unpatched/older Android builds will have several issues in trying to handle them, like:
So it is sort of messy. |
Word. Basically an invitation for 1-star reviews, unless handled somewhat gracefully in advance :) Thanks for clarifing that side of things @Codelica . |
… app in background or killed (#29659) # Why The logic in `onNotificationResponseFromExtras` needs to be adjusted to correctly check whether response listeners are active, and do that check first before falling back to add the response to the pending responses (eventually used by `useLastNotificationResponse()` in JS). See #28656 (comment) # How Inverted the order of checks in the method. # Test Plan - CI should pass - Test the flows with the test app, using both expo-server-sdk tool and expo.dev/notifications web tool # Checklist <!-- 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).
Thanks @Codelica for confirming that it will still "work" but with terrible experience like @nakedgun said :) at least iOS I hope will work fine... I am just debating maybe it's ok for it to work badly like this for a few extra days while the the |
There is an issue after updating to the latest version. When using useFcmV1=true in the Master App for push notifications:
|
… app in background or killed (#29659) # Why The logic in `onNotificationResponseFromExtras` needs to be adjusted to correctly check whether response listeners are active, and do that check first before falling back to add the response to the pending responses (eventually used by `useLastNotificationResponse()` in JS). See #28656 (comment) # How Inverted the order of checks in the method. # Test Plan - CI should pass - Test the flows with the test app, using both expo-server-sdk tool and expo.dev/notifications web tool # Checklist <!-- 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).
@codercoder292, please check out my demo repo which applies the last patch needed for this to work https://github.com/mgscreativa/expo-notifications-test |
Just tested expo-notifications@0.28.9 and it works just fine. Updated test repo here https://github.com/mgscreativa/expo-notifications-test useFcmV1=true
|
…e legacy icon and color (#29491) Based on customer feedback from `expo-notifications@0.28.7`, three adjustments are still needed: - Set both FCM legacy and FCMv1 metadata items in the Android manifest, so that icon and color work in both cases - Add a config plugin property, `defaultChannel`, to allow a developer to set the FCMv1 default channel in the manifest. - The Android lifecycle listeners should check to see if the intent extras have a `notificationResponse` object, and not map the intent bundle to create a duplicate response in JS. See [this comment](#28656 (comment)) by @mgscreativa and other comments in that issue. - Make the appropriate code changes in the plugin and in `ExpoNotificationLifecycleListener.java` - CI should pass - Test app patched with these changes should behave as expected <!-- 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).
… app in background or killed (#29659) The logic in `onNotificationResponseFromExtras` needs to be adjusted to correctly check whether response listeners are active, and do that check first before falling back to add the response to the pending responses (eventually used by `useLastNotificationResponse()` in JS). See #28656 (comment) Inverted the order of checks in the method. - CI should pass - Test the flows with the test app, using both expo-server-sdk tool and expo.dev/notifications web tool <!-- 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).
I am facing the same issue migrating to fcm v1, I found out that the notification that I get from If you want to quickfix it, you can do something like:
|
@codercoder292 and @NoZiL are you doing a fresh build after updating to expo-notifications 0.28.8? The symptoms described (like banner not appearing, missing/unparsed data) all required native code fixes. A few suggestions:
[
"expo-notifications",
{
"icon": "./assets/images/android-notification.png",
"color": "#D84F4F",
"defaultChannel": "default"
}
]
|
expo-notifications is at v0.28.9 at the moment |
This is a tracking issue for user-reported issues related to the Spring 2024 migration of Expo Push Notifications from FCM Legacy to FCM V1 for Android devices. There have been reported instances of brokenness in production when moving from FCM Legacy to FCM V1, and this issue will serve as a central place to track our investigation and resolution of these issues.
The known issues are as follows:
data
payload in client when receiving push notificationsexpo-notifications@0.28.4
expo-notifications@0.28.5
useLastNotificationResponse()
has null dataHere is a table showing the overall state of these issues, as of 6/11. Package versions are expo@51.0.11 and expo-notifications@0.28.8:
New issues filed in expo/expo will be merged into this tracking issue for easier management. The deadline for migrating to FCM V1 is June 20.
The text was updated successfully, but these errors were encountered: