feat(ios, messaging): Allow notifications in foreground on iOS, configure in firebase.json #6407

ken0nek · 2022-07-18T22:12:21Z

Description

Resolve this TODO comment ("in a later version allow customising completion options in JS code").

Added a key to configure UNNotificationPresentationOptions

UNNotificationPresentationOptions | Apple Developer Documentation

Related issues

🔥[🐛] Messaging: willPresentNotification delegate has no effect on iOS · Issue #5884 · invertase/react-native-firebase
feat(ios): Add banner and list options to IOSForegroundPresentationOptions by ken0nek · Pull Request #463 · invertase/notifee
feat(ios): Add iOS 14 compatibility for alert by ken0nek · Pull Request #465 · invertase/notifee
feat: deprecate foregroundPresentationOptions.alert by helenaford · Pull Request #467 · invertase/notifee

Release Summary

Can configure UNNotificationPresentationOptions on iOS via firebase.json

Checklist

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

Test Plan

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

👉 Star this repo on GitHub ⭐️
👉 Follow React Native Firebase and Invertase on Twitter

vercel · 2022-07-18T22:12:26Z

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

Name	Status	Preview	Updated
react-native-firebase	✅ Ready (Inspect)	Visit Preview	Jul 19, 2022 at 4:35AM (UTC)

1 Ignored Deployment

Name	Status	Preview	Updated
react-native-firebase-next	⬜️ Ignored (Inspect)		Jul 19, 2022 at 4:35AM (UTC)

ken0nek · 2022-07-18T22:28:54Z

packages/messaging/ios/RNFBMessaging/RNFBMessaging+UNUserNotificationCenter.m

+  // if list or banner is true, ignore `alert` property
+  if (banner || list) {
+    if (banner) {
+      if (@available(iOS 14, *)) {
+        presentationOptions |= UNNotificationPresentationOptionBanner;
+      } else {
+        // for iOS 13 we need to set `alert`
+        presentationOptions |= UNNotificationPresentationOptionAlert;
+      }
+    }
+
+    if (list) {
+      if (@available(iOS 14, *)) {
+        presentationOptions |= UNNotificationPresentationOptionList;
+      } else {
+        // for iOS 13 we need to set `alert`
+        presentationOptions |= UNNotificationPresentationOptionAlert;
+      }
+    }
+  } else if (alert) {
+    // TODO: Remove `alert` once iOS 14 becomes the minimum deployment target
+    presentationOptions |= UNNotificationPresentationOptionAlert;
+  }


Same logic in notifee

yes - I was silently reading those as they went through, so I followed things as they went to the final solution, this looks good to me

mikehardy

Looks good to me, thanks a lot for this! Normally I tell people to use Notifee for device-local-anything when it comes to notification presentation but including at least a minimum for foreground (similar to background) is very nice.

One quick question: is there a feature gap on the android side as well for notifications showing up in the default state?

mikehardy · 2022-07-19T00:09:43Z

packages/messaging/ios/RNFBMessaging/RNFBMessaging+UNUserNotificationCenter.m

+  // if list or banner is true, ignore `alert` property
+  if (banner || list) {
+    if (banner) {
+      if (@available(iOS 14, *)) {
+        presentationOptions |= UNNotificationPresentationOptionBanner;
+      } else {
+        // for iOS 13 we need to set `alert`
+        presentationOptions |= UNNotificationPresentationOptionAlert;
+      }
+    }
+
+    if (list) {
+      if (@available(iOS 14, *)) {
+        presentationOptions |= UNNotificationPresentationOptionList;
+      } else {
+        // for iOS 13 we need to set `alert`
+        presentationOptions |= UNNotificationPresentationOptionAlert;
+      }
+    }
+  } else if (alert) {
+    // TODO: Remove `alert` once iOS 14 becomes the minimum deployment target
+    presentationOptions |= UNNotificationPresentationOptionAlert;
+  }


yes - I was silently reading those as they went through, so I followed things as they went to the final solution, this looks good to me

codecov · 2022-07-19T00:16:01Z

Codecov Report

Merging #6407 (01fe662) into main (254f9fc) will decrease coverage by 46.48%.
The diff coverage is n/a.

❗ Current head 01fe662 differs from pull request most recent head 027192c. Consider uploading reports for the commit 027192c to get more accurate results

@@             Coverage Diff             @@
##             main    #6407       +/-   ##
===========================================
- Coverage   72.31%   25.84%   -46.47%     
===========================================
  Files         109       97       -12     
  Lines        4655     4289      -366     
  Branches     1048     1048               
===========================================
- Hits         3366     1108     -2258     
- Misses       1211     2581     +1370     
- Partials       78      600      +522

ken0nek · 2022-07-19T02:12:26Z

One quick question: is there a feature gap on the android side as well for notifications showing up in the default state?

I suppose no.

As you can see this doc, foreground presentation options for Remote Notification is only for iOS.

ken0nek · 2022-07-19T02:15:47Z

Hmm, I got no diff even after executing yarn lint:markdown 🤔

mikehardy · 2022-07-19T02:36:26Z

I suppose no.

As you can see this doc, foreground presentation options for Remote Notification is only for iOS.

Hmm - I think of Notifee as going "higher" in the stack, we are below them (in my mental model - that is, lower level) and firebase-android-sdk would be even lower than that, with the GCM libraries and FCM receipt on the device as the bottom / most inner part.

My question was aimed at: as GCM/FCM hits the device and firebase-android-sdk executes things / potentially booting the app and loading the Java code then the javascript bundle (similar to firebase-ios-sdk loading the objective-c code in @react-native-firebase/messaging that you changed) - if the app is in the foreground is a notification actually posted ever? Should it be possible in the way you added this feature here?

I have not actually tested this (at least not recently enough to remember) so I'm unsure - I was mostly curious for the results of any testing if you've done it. I'm okay with just doing an iOS feature and waiting for someone interested (or myself) to do same for android if there's a feature gap, I was just curious if you knew since I assume you're testing this with a react-native app so you looked at iOS and could probably check android with same codebase / testing setup

mikehardy · 2022-07-19T02:38:26Z

Hmm, I got no diff even after executing yarn lint:markdown thinking

I see this:

mike@bistromath:~/work/Invertase/react-native-firebase (work/UNNotificationPresentationOption-config) % yarn lint:markdown -w
yarn run v1.22.18
$ prettier --check "docs/**/*.md" -w
Checking formatting...
[warn] docs/messaging/usage/index.md
[warn] Code style issues fixed in the above file.
Done in 3.44s.
mike@bistromath:~/work/Invertase/react-native-firebase (work/UNNotificationPresentationOption-config *) % git diff
diff --git a/docs/messaging/usage/index.md b/docs/messaging/usage/index.md
index a93f73e67..1383dd45a 100644
--- a/docs/messaging/usage/index.md
+++ b/docs/messaging/usage/index.md
@@ -395,13 +395,7 @@ Refer to [UNNotificationPresentationOptions](https://developer.apple.com/documen
 // <projectRoot>/firebase.json
 {
   "react-native": {
-    "messaging_ios_foreground_presentation_options": [
-      "badge",
-      "sound",
-      "alert",
-      "list",
-      "banner"
-    ]
+    "messaging_ios_foreground_presentation_options": ["badge", "sound", "alert", "list", "banner"]
   }
 }

mikehardy · 2022-07-19T02:39:34Z

(I pushed a commit with that diff to your branch as an attempt at higher velocity - hopefully no offense is taken! :-) )

ken0nek · 2022-07-19T03:25:50Z

Thank you for updating the doc!

I was mostly curious for the results of any testing if you've done it.
I was just curious if you knew since I assume you're testing this with a react-native app so you looked at iOS

I tested this feature by patching @react-native-firebase/messaging and @react-native-firebase/app in my company codebase. It works on iOS by sending a remote push notification, plus I got a notification on Android too.

I'm okay with just doing an iOS feature and waiting for someone interested (or myself) to do same for android if there's a feature gap

Maybe we can explore the code around onMessageReceived on Android in the future?

.spellcheck.dict.txt

mikehardy

looks even better now, thanks for organizing + patching up the spellcheck dictionary

mikehardy · 2022-07-19T04:23:40Z

🤔 perhaps this collided with #6406 and this needs a rebase against current main ?

alphabetically

ken0nek · 2022-07-19T04:30:30Z

🤔 perhaps this collided with #6406 and this needs a rebase against current main ?

Rebased using the latest main 👍

mikehardy · 2022-07-19T04:31:26Z

I'll be shocked if this doesn't do it - I'm about to be done for the night so won't see the result for hours but I imagine it will be either all green or perhaps a flaky-CI-test or something. But it all looks good now to me

ken0nek changed the title ~~Work/un notification presentation option config~~ Configure foreground presentation options on iOS via firebase.json Jul 18, 2022

vercel bot deployed to Preview – react-native-firebase July 18, 2022 22:17 View deployment

ken0nek changed the title ~~Configure foreground presentation options on iOS via firebase.json~~ fix(ios, messaging): Configure foreground presentation options on iOS via firebase.json Jul 18, 2022

ken0nek commented Jul 18, 2022

View reviewed changes

mikehardy changed the title ~~fix(ios, messaging): Configure foreground presentation options on iOS via firebase.json~~ feat(ios, messaging): Allow notifications in foreground on iOS, configure in firebase.json Jul 19, 2022

mikehardy previously approved these changes Jul 19, 2022

View reviewed changes

mikehardy added Workflow: Waiting for User Response Blocked waiting for user response. Workflow: Pending Merge Waiting on CI or similar labels Jul 19, 2022

ken0nek dismissed mikehardy’s stale review via e14e403 July 19, 2022 02:15

vercel bot deployed to Preview – react-native-firebase July 19, 2022 02:20 View deployment

vercel bot deployed to Preview – react-native-firebase July 19, 2022 02:44 View deployment

ken0nek commented Jul 19, 2022

View reviewed changes

.spellcheck.dict.txt Show resolved Hide resolved

.spellcheck.dict.txt Show resolved Hide resolved

vercel bot deployed to Preview – react-native-firebase July 19, 2022 03:36 View deployment

mikehardy previously approved these changes Jul 19, 2022

View reviewed changes

ken0nek and others added 7 commits July 18, 2022 21:26

Add an option to configure foreground presentation option on iOS

ce47497

Fix a fallback logic for list and banner

ac8cdc2

Add description for messaging_ios_foreground_presentation_options

fe67e5a

yarn lint:ios:fix

d1542cb

yarn lint:markdown -w

d77abc1

Add UNNotificationPresentationOptions to spellcheck and sort

e2676be

alphabetically

Update the doc not to use alert

027192c

ken0nek dismissed mikehardy’s stale review via 027192c July 19, 2022 04:29

ken0nek force-pushed the work/UNNotificationPresentationOption-config branch from def4def to 027192c Compare July 19, 2022 04:29

mikehardy removed the Workflow: Waiting for User Response Blocked waiting for user response. label Jul 19, 2022

vercel bot deployed to Preview – react-native-firebase July 19, 2022 04:35 View deployment

mikehardy merged commit 71dee2b into invertase:main Jul 19, 2022

mikehardy removed the Workflow: Pending Merge Waiting on CI or similar label Jul 19, 2022

ken0nek deleted the work/UNNotificationPresentationOption-config branch September 8, 2022 18:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(ios, messaging): Allow notifications in foreground on iOS, configure in firebase.json #6407

feat(ios, messaging): Allow notifications in foreground on iOS, configure in firebase.json #6407

ken0nek commented Jul 18, 2022

vercel bot commented Jul 18, 2022 •

edited

Loading

ken0nek Jul 18, 2022

mikehardy Jul 19, 2022

mikehardy left a comment

mikehardy Jul 19, 2022

codecov bot commented Jul 19, 2022 •

edited

Loading

ken0nek commented Jul 19, 2022

ken0nek commented Jul 19, 2022

mikehardy commented Jul 19, 2022

mikehardy commented Jul 19, 2022

mikehardy commented Jul 19, 2022

ken0nek commented Jul 19, 2022 •

edited

Loading

mikehardy left a comment

mikehardy commented Jul 19, 2022

ken0nek commented Jul 19, 2022 •

edited

Loading

mikehardy commented Jul 19, 2022

feat(ios, messaging): Allow notifications in foreground on iOS, configure in firebase.json #6407

feat(ios, messaging): Allow notifications in foreground on iOS, configure in firebase.json #6407

Conversation

ken0nek commented Jul 18, 2022

Description

Related issues

Release Summary

Checklist

Test Plan

vercel bot commented Jul 18, 2022 • edited Loading

ken0nek Jul 18, 2022

Choose a reason for hiding this comment

mikehardy Jul 19, 2022

Choose a reason for hiding this comment

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy Jul 19, 2022

Choose a reason for hiding this comment

codecov bot commented Jul 19, 2022 • edited Loading

Codecov Report

ken0nek commented Jul 19, 2022

ken0nek commented Jul 19, 2022

mikehardy commented Jul 19, 2022

mikehardy commented Jul 19, 2022

mikehardy commented Jul 19, 2022

ken0nek commented Jul 19, 2022 • edited Loading

mikehardy left a comment

Choose a reason for hiding this comment

mikehardy commented Jul 19, 2022

ken0nek commented Jul 19, 2022 • edited Loading

mikehardy commented Jul 19, 2022

vercel bot commented Jul 18, 2022 •

edited

Loading

codecov bot commented Jul 19, 2022 •

edited

Loading

ken0nek commented Jul 19, 2022 •

edited

Loading

ken0nek commented Jul 19, 2022 •

edited

Loading