Merge expo-notifications FCMv1 fixes to SDK 50 #29714
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Why Fixes events not being sent to js because native code uses the incorrect emitter. Closes ENG-11898. # Test plan - https://github.com/douglowder/NotificationsTest/tree/canary ✅
…background or not running (#28883)
…useLastNotificationResponse hook (#28938)
- Fixes #24844 - Corrects an NPE that can happen if a deep link in the app is opened - Change constants in the `expo-notifications` plugin to the correct ones expected for FCMv1 - Add the recommended code to guard against NPE when logging Android lifecycle events - CI should pass - Test app should show the correct icon and color in notification banners <!-- 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).
After the fixes in #29204 , there are still reports of NPEs happening in the notification lifecycle listener methods. These NPEs are coming from code that causes Android to log events. Remove the logging code that was causing the issue. - CI should pass - Test app should work, show correct icons in notifications, and responses should show up even if app is killed - [ ] 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).
Remove a console.log call that was logging notification data. 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).
…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).
… 0 (#29657) # Why The `ShortcutBadger` package does not correctly set the badge count on Android if the value to be set is 0. This fixes the issue, using a fix provided by @haileyok in #29634 . Fixes #29634. # How In the case where badge count is 0, use Android built in notification manager to cancel all notifications. # Test Plan - CI should pass - API should behave correctly in our test app # 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).
… 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).
douglowder
requested review from
Simek,
amandeepmittal and
tsapeta
as code owners
|
Set up a branch of the microfoam test app for SDK 50 and built it with these changes. Tests with FCMv1:
|
|
Would be nice to regenerate API documentation data JSON for SDK 50 from the |
tsapeta
approved these changes
Jun 13, 2024
lukmccall
approved these changes
Jun 13, 2024
christopherwalter
approved these changes
Jun 13, 2024
|
@Simek we are making no changes to the generated API data, only to the |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why
We would like to provide the recent FCMv1 fixes for expo-notifications to SDK 50 customers, if possible.
How
Cherry picked the recent changes in expo-notifications from SDK 51.
This PR is to provide extra review and testing, before we actually merge these changes to SDK 50 and publish.
Test Plan
Checklist
npx expo prebuild& EAS Build (eg: updated a module plugin).