feat: update FCM and Notification Services to better support push impl #13441
Conversation
clean up the interface, and add comments and methods for parts we actually want to expose
add better tests, and correctly handle notification data payload strings
|
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
| /** | ||
| * @todo - update controller to get up-to-date state | ||
| */ |
There was a problem hiding this comment.
TODO - we need to publish the new controller changes. But this is not breaking if we do merge this in.
|
|
||
| /** | ||
| * Registers Push Notifications | ||
| * @todo - clean up props once we finalise integration |
There was a problem hiding this comment.
To reduce codeowners, we've kept the same interface.
We'll create a separate PR to remove these unused props.
| // We only subscribe to foreground messages, as subscribing to background messages that contain `notification` + `data` payloads have issues | ||
| // IOS - requires payload editing (https://notifee.app/react-native/docs/ios/remote-notification-support) | ||
| // IOS - requires isHeadless injection and app modification to ship a minimal app when headless (https://rnfirebase.io/messaging/usage#background-application-state). | ||
| // Android - will cause double notifications if a remote message contains both `notification` + `data` payloads | ||
| // Firebase will still send push notifications in background + app kill as there is a `notification` payload in the remote message | ||
| await this.#registerForegroundMessages(handler); |
There was a problem hiding this comment.
Yeah... there is a lot of edge cases with push notifications across platforms.
Keeping this lengthy comment for future devs/me.
| @@ -30,23 +29,29 @@ class NotificationsService { | |||
| async getBlockedNotifications(): Promise<Map<ChannelId, boolean>> { | |||
There was a problem hiding this comment.
TBH most of this was designed before and I don't have much context around it.
There may be some stuff here that is not needed. But we'll keep this for now.
|
quality gate spotted some good finds, and also not useful comments. This should resolve the issues
Prithpal-Sooriya
added
Run Smoke E2E
|
Prithpal-Sooriya
added
Run Smoke E2E
Prithpal-Sooriya
added
Run Smoke E2E
|
|
Prithpal-Sooriya
added
Run Smoke E2E
|
Description
This is another partial PR from our large draft feature branch: #13006
We have 1 (or 2) more PRs to do the integration and then push notifications are in!
Related issues
Fixes:
Manual testing steps
No testing steps yet, this PR is not (yet) testable as the feature is not released and incomplete.
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist