Skip to content
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

fix(ios): Remote issues: #1041 / #1096 onForegroundEvent() and getInitialNotification() #1118

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

LunatiqueCoder
Copy link

@LunatiqueCoder LunatiqueCoder commented Oct 8, 2024

👋 Hello!

This PR fixes

🙏 Please keep in mind I just learned Objective C and Swift in order to fix those issues.

What's changed?

How to test

  1. Be sure to configure Notifee to handle remote notifications
  2. If you're using patch-package, in your patches folder create a file called @notifee+react-native+9.1.1.patch
  3. Paste the following:
diff --git a/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+NSNotificationCenter.m b/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+NSNotificationCenter.m
index 560a62c..c010367 100644
--- a/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+NSNotificationCenter.m
+++ b/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+NSNotificationCenter.m
@@ -58,13 +58,18 @@
 #pragma mark Application Notifications
 
 - (void)application_onDidFinishLaunchingNotification:(nonnull NSNotification *)notification {
-#pragma clang diagnostic push
-#pragma clang diagnostic ignored "-Wdeprecated-declarations"
-  UILocalNotification *launchNotification =
-      (UILocalNotification *)notification.userInfo[UIApplicationLaunchOptionsLocalNotificationKey];
-  [[NotifeeCoreUNUserNotificationCenter instance]
-      onDidFinishLaunchingNotification:launchNotification.userInfo];
-  [[NotifeeCoreUNUserNotificationCenter instance] getInitialNotification];
+  NSDictionary *notifUserInfo =
+      notification.userInfo[UIApplicationLaunchOptionsLocalNotificationKey];
+    
+  if (!notifUserInfo) {
+      // Fallback to remote notification key if local notification key is not available
+      notifUserInfo = notification.userInfo[UIApplicationLaunchOptionsRemoteNotificationKey];
+  }
+
+  if (notifUserInfo) {
+      [[NotifeeCoreUNUserNotificationCenter instance] onDidFinishLaunchingNotification:notifUserInfo];
+      [[NotifeeCoreUNUserNotificationCenter instance] getInitialNotification];
+  }
 
   [[NotifeeCoreUNUserNotificationCenter instance] observe];
 }
diff --git a/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+UNUserNotificationCenter.m b/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+UNUserNotificationCenter.m
index beaa360..ff53bef 100644
--- a/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+UNUserNotificationCenter.m
+++ b/node_modules/@notifee/react-native/ios/NotifeeCore/NotifeeCore+UNUserNotificationCenter.m
@@ -21,7 +21,6 @@
 #import "NotifeeCoreUtil.h"
 
 @implementation NotifeeCoreUNUserNotificationCenter
-
 struct {
   unsigned int willPresentNotification : 1;
   unsigned int didReceiveNotificationResponse : 1;
@@ -73,12 +72,31 @@ struct {
 
 - (nullable NSDictionary *)getInitialNotification {
   if (_initialNotificationGathered && _initialNotificationBlock != nil) {
+
     // copying initial notification
     if (_initialNotification != nil &&
         [_initialNoticationID isEqualToString:_notificationOpenedAppID]) {
-      NSDictionary *initialNotificationCopy = [_initialNotification copy];
+
+      NSMutableDictionary *event = [NSMutableDictionary dictionary];
+      NSMutableDictionary *initialNotificationCopy = [_initialNotification mutableCopy];
+      initialNotificationCopy[@"initialNotification"] = @1;
+
       _initialNotification = nil;
-      _initialNotificationBlock(nil, initialNotificationCopy);
+
+      // Sets the return payload for getInitialNotification()
+     _initialNotificationBlock(nil, initialNotificationCopy);
+
+      // Prepare onForegroundEvent() payload
+      event[@"detail"] = [initialNotificationCopy copy];
+      if ([event[@"detail"][@"pressAction"][@"id"] isEqualToString:@"default"]) {
+          event[@"type"] = @1;  // PRESS
+      } else {
+          event[@"type"] = @2;  // ACTION_PRESS
+      }
+
+      // Call onForegroundEvent() on Javascript side with payload:
+      [[NotifeeCoreDelegateHolder instance] didReceiveNotifeeCoreEvent:event];
+
     } else {
       _initialNotificationBlock(nil, nil);
     }
@@ -104,7 +122,6 @@ struct {
   NSDictionary *notifeeNotification =
       notification.request.content.userInfo[kNotifeeUserInfoNotification];
 
-  // we only care about notifications created through notifee
   if (notifeeNotification != nil) {
     UNNotificationPresentationOptions presentationOptions = UNNotificationPresentationOptionNone;
     NSDictionary *foregroundPresentationOptions =
@@ -148,23 +165,16 @@ struct {
       presentationOptions |= UNNotificationPresentationOptionAlert;
     }
 
-    NSDictionary *notifeeTrigger = notification.request.content.userInfo[kNotifeeUserInfoTrigger];
-    if (notifeeTrigger != nil) {
-      // post DELIVERED event
-      [[NotifeeCoreDelegateHolder instance] didReceiveNotifeeCoreEvent:@{
+    // post DELIVERED event
+    [[NotifeeCoreDelegateHolder instance] didReceiveNotifeeCoreEvent:@{
         @"type" : @(NotifeeCoreEventTypeDelivered),
         @"detail" : @{
           @"notification" : notifeeNotification,
         }
-      }];
-    }
+    }];
 
     completionHandler(presentationOptions);
 
-  } else if (_originalDelegate != nil && originalUNCDelegateRespondsTo.willPresentNotification) {
-    [_originalDelegate userNotificationCenter:center
-                      willPresentNotification:notification
-                        withCompletionHandler:completionHandler];
   }
 }
 

@LunatiqueCoder LunatiqueCoder changed the title bug/#1109-#1096-remote-onForegroundEvent-getInitialNotification fix(ios): Remote issues: #1109-#1096 onForegroundEvent() and getInitialNotification() Oct 8, 2024
…remote-onForegroundEvent-getInitialNotification' into bug/invertase#1109-invertase#1096-remote-onForegroundEvent-getInitialNotification
@mikehardy
Copy link
Contributor

Hey @LunatiqueCoder 👋 ! Haven't hacked on things with you in a while, good to see you

I need to focus first on making sure Notifee is compatible with react-native 0.76 as I believe we have New Architecture incompatibilities even with the compat mode

But right after that I was planning on tackling all of these issues related to FCM delivery and events not firing and such - and it looks like you have beaten me to it with this PR

This is an area I really only want to change once (or...one more time) though so I need to think pretty deeply about how exactly this should work (should it consume the initial notification or not? how exactly should it interoperate with react-native-firebase? etc). So please be patient as it may be a little bit before I get to this, but it is pretty important to get right and I'm excited to see you've already thought through it with this patch here

@mikehardy
Copy link
Contributor

For one specific question:

This will also enable getInitialNotification() for iOS if the app was opened from a terminated state by pressing the notification. However, the docs say it should be deprecated. How are we supposed to handle this scenario without getInitialNotification()? Should we remove that part from the docs?

The app is started and brought to the foreground and I believe a Notifee event handler is called, to deliver the event?

@LunatiqueCoder
Copy link
Author

LunatiqueCoder commented Oct 8, 2024

@mikehardy 🍺 Hehe, long time indeed. Happy to talk to you again

Click to see devs chit chatI 'm still tinkering, but I was very excited to see I might have fixed some Github issues while I was working on my stuff, especially when I saw you're a core contributor here as well.

I'm also planning to open source an Expo config plugin for Notifee's iOS remote notifications. Basically it follows these steps here: https://notifee.app/react-native/docs/ios/remote-notification-support


Anyway, back to our issues:


For one specific question:

This will also enable getInitialNotification() for iOS if the app was opened from a terminated state by pressing the notification. However, the docs say it should be deprecated. How are we supposed to handle this scenario without getInitialNotification()? Should we remove that part from the docs?

The app is started and brought to the foreground and I believe a Notifee event handler is called, to deliver the event?


EDIT: ☝️ Updated first post with answer


As a side note, since apns-push-type: background it's almost useless, we had to rely on remote notifications for iOS without breaking Android.

We ended up using this following snippet with Firebase Node.js SDK
which I think it would be very useful to add in the docs):

import type {NextApiRequest, NextApiResponse} from 'next';
import type {Notification} from '@notifee/react-native/src/types/Notification';
import {AndroidImportance} from '@notifee/react-native/src/types/NotificationAndroid';
import {MulticastMessage} from 'firebase-admin/lib/messaging/messaging-api';


const notifeeOptions: Notification = {
  id: Date.now().toString(),
  title: 'Title',
  subtitle: 'Subtitle',
  body: 'Main body content of the notification',
  android: {
    channelId: 'default',
    importance: AndroidImportance.HIGH,
    pressAction: 
  },
  ios: {
    sound: 'default',
    criticalVolume: 1,
    critical: true,
    foregroundPresentationOptions: {
      badge: true,
      banner: true,
      list: true,
      sound: true,
    },
  },
};



const message: MulticastMessage = {
  // ✅ We can continue using local notifcation for Android
  // 👍 while triggering iOS remote notifications from `apns`
  data: {notifee_options: JSON.stringify(notifeeOptions)}, 
  tokens: [],
  android: {
    priority: 'high',
  },
  apns: {
    payload: {
      notifee_options: notifeeOptions,
      aps: {
        alert: {
          // 🚧 This is needed to trigger an alert/remote notification only for iOS
          // 👍 but Android will continue using data-only notifications
          title: 'Hello World',
        },
        mutableContent: true,
      },
    },
  },
};

export default async (req: NextApiRequest, res: NextApiResponse) => {
  try {
    await admin.messaging().sendEachForMulticast(message);

    res.status(200).end();
  } catch (e) {
    res.status(400).end();
  }
};

@dprevost-LMI
Copy link

dprevost-LMI commented Oct 8, 2024

FYI, I just tested the fix, and on my side, the first index in the patch differed from mine.

Also, after some JS debugging, the call to await notified.getInitialNotification(); was not returning at all, aka the promise was hanging there.
I'm not an iOS expert, so something else I'm not aware from our project could interfere!

@LunatiqueCoder
Copy link
Author

Hmm, just noticed those changes were made with @notifee/[email protected]. If I update to the latest version, the patch fails. I'll try again and see how it goes. Thanks for the input!

@dprevost-LMI
Copy link

I'm also on "@notifee/[email protected]":

…Notification - call onForegroundEvent() when app starts from terminated state - same as getInitialNotification()
Copy link
Author

@LunatiqueCoder LunatiqueCoder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Leaving a note for myself. From my testing, onForeground gets called twice for DELIVERED events on iOS

}

// Call onForegroundEvent() on Javascript side with payload:
[[NotifeeCoreDelegateHolder instance] didReceiveNotifeeCoreEvent:event];
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚧 Update:

I found another funny issue with the patch :( But should be an easy fix. This is the workaround however:

useEffect(() => {
    /** 🚧 Bug with Notifee patch
     * if we don't call this, we don't get onForegroundEvent if app was opened
     * from a terminated state by pressing notification
     * @todo Remove once this is fixed:
     * @link https://github.com/invertase/notifee/pull/1118 */
    notifee.getInitialNotification();

    const unsubscribe = notifee.onForegroundEvent(async ({ type, detail }) => {

    });

    return unsubscribe;
  }, [dispatch]);
};

@mikehardy mikehardy changed the title fix(ios): Remote issues: #1109-#1096 onForegroundEvent() and getInitialNotification() fix(ios): Remote issues: #1041 / #1096 onForegroundEvent() and getInitialNotification() Oct 18, 2024
@mikehardy mikehardy added ios Workflow: Needs Review Needs a maintainer to have a look labels Oct 18, 2024
@mateoabrbt
Copy link

Amazing work hope to see news about this PR soon 👀 thanks !

@shonie2xx
Copy link

shonie2xx commented Oct 21, 2024

Did I just read "just learned Objective C and Swift in order to fix those issues." Great stuff man, looking forward to see this PR merged, thanks!

@TestWts2017
Copy link

when we can expect the latest version with the updated code?

@mikehardy
Copy link
Contributor

@TestWts2017 you can't. It is open source and I am always very clear with people - when you interact with open source you must abandon your expectations of release dates. If it is important to you, use patch-package and integrate the changes in your project directly, no need to wait

@LunatiqueCoder
Copy link
Author

☝️ the referenced issues above might be duplicated

@mikehardy
Copy link
Contributor

They probably are, most of the complaints in this repo are all actually the same complaint / disagreement with how it functions, or some ridiculously niche thing which is hard to solve 😆

Been traveling all week but will be back to work next week

@mateoabrbt
Copy link

mateoabrbt commented Oct 28, 2024

Hey there! I partly agree with this because the Notifee documentation can be pretty unclear, especially when it comes to integration. It often feels like you have to dig through issues or PRs to get answers. Also, I wonder why Invertase promotes Notifee in react-native-firebase if it’s considered niche? Notifee is such a powerful library and a great addition to any React Native project. Ideally, it should work seamlessly on both iOS and Android with Firebase—if not, maybe it shouldn’t be featured as an option!

@mikehardy
Copy link
Contributor

mikehardy commented Oct 28, 2024

Just to answer one part: Notifee is not at all considered niche! However, a full screen notification is a niche case, as is a permanent notification attached to a foreground service of a niche type, as is scheduling exact alarms. Each of those is a niche feature just based on query traffic here related to them. Non-niche use is "I want to receive an FCM and post a notification, perhaps with styling and an image, in the foreground and background case"

It just so happens that this PR is right in that area of work, which is great. But also why it needs careful consideration as it will affect the majority case.

However, my mention of the other stuff as niche is not to say Notifee shouldn't be supporting it. And in either case, react-native-firebase had to eject the local-notification device API surface area specifically because all those niche use cases (which are each important to someone!) were a huge drag on the core purpose of react-native-firebase which is to wrap firebase APIs.

It is not possible (we tried!) to support the local device notification APIs people want and all the firebase APIs in the react-native-firebase repos, we had to split it out, thus Notifee exists.

And unfortunately, with the pace of device-local notification API change (typically in the form of increasing restrictions to curb abusive behavior from app developers...) and the pace of change of device-local power management which affects message delivery (typically in the form of increasing restrictions to curb abusive behavior from app developers...) there is no simple integration. Troubleshooting unfortunately always starts with a tree of questions of the form "okay, iOS or Android? okay is it a notification, data, or mixed FCM? okay, what version of the operating system?" and gets worse from there.

There is no seemless unfortunately. The work in this PR should make it a great deal better though because yes, it is rough going right now.

Hope that's sort of clear with regard to maintainer goals and repo motivation at least?

@mateoabrbt
Copy link

I completely agree that separating Notifee from react-native-firebase was the right approach! I also understand the difficulty of meeting OS requirements for both Android and iOS—each has its own set of complexities. Over the past few months, I’ve read through many Notifee issues and comments to better understand how notifications work across platforms, and I want to acknowledge and appreciate all the hard work put into this package.

I’m excited to see the improvements in the PR, as they’ll make these features more accessible. That said, I know that adding new functionality without impacting other areas is a significant challenge, especially given the wide range of use cases Notifee supports. Enhanced documentation, particularly around Firebase integration, could also help more developers avoid common pitfalls and make setup easier. I’ve noticed that this repo used to get frequent responses from you and other maintainers, so better documentation could help lighten that load too.

Thanks for the insight—it really clarifies the goals and challenges behind Notifee!

@mikehardy
Copy link
Contributor

I know there is a lot of interest in this one and I am as well. Just wanted to let everyone know it won't sit forever but I'm still backlogged on urgent items - that is there is one repo left that still does not seem to be behaving well with new architecture at all in the form of @invertase/react-native-apple-authentication and I must get that working first. cocoapods/xcodeproj blowing up all react-native-firebase builds last week was unexpected and took time as well.

There is one other new architecture issue here that came up where foreground services appear to freeze and I'll need to handle that as a regression before coming back to this, but once all the new arch stuff is done, this is first. It will happen but I'll appreciate the patience in the meantime

@mikehardy
Copy link
Contributor

Getting closer on this - apple auth qualified on new arch, and got reproduction scaffolding setup in react-native-firebase in order to test this stuff over the weekend invertase/react-native-firebase#8143 - there is methodical progress towards having real test coverage for all the foreground / background stuff. I continue to appreciate the patience

@dhruvvbhavsar
Copy link

Thanks man! This fix really helped!!!!
Keep doing the good man's work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ios Workflow: Needs Review Needs a maintainer to have a look
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants