invertase / notifee

βš›οΈ A feature rich notifications library for React Native.
https://notifee.app
Apache License 2.0
1.83k stars 214 forks source link

Notifee should not break React Native Firebase #912

Open go-sean-go opened 10 months ago

go-sean-go commented 10 months ago

Notifee is, per its docs, "a library for React Native, bringing local notification support to both Android & iOS applications."

Yet it purposefully and knowingly breaks React Native Firebase's remote notification tap handlers in v7.x (link):

onNotificationOpenedApp and getInitialNotification from RNFB Messaging will no longer trigger as Notifee will handle the event. Should not require any code changes to these RNFB event handlers, as events on Android will continue to work as normal

onNotificationOpenedApp and getInitialNotification are effectively required to do anything interesting with remote notifications. It's difficult for me to imagine any serious app using remote notifications without using them.

I'm not familiar with the intricacies of this decision, but to me the stated goal ("This allows quick actions from remote notifications to be supported without the need of a NSE") does not nearly justify the change. Handling quick actions is rare, whereas handling simple taps is extremely common (basically table stakes). This change purposefully and knowingly breaks an adjacent library irreparably in exchange for a slightly simpler implementation of itself.

There was also a simple solution requested back in April, here: https://github.com/invertase/notifee/issues/755

I would like to see a functionality where I can turn that off in notifee, something like:

notifee.setRemoteHandling(false);

I agree with it - except that I feel the more proper solution is to flip it around: notifee should NOT break other libraries by default. It should be opt-in, i.e. something like this:

To handle remote notifications with Notifee, use notifee.setRemoteHandling(true);

NOTE: If you use React Native Firebase's Messaging library, by setting remote handling to true, onNotificationOpenedApp and getInitialNotification will no longer trigger.

Again, Notifee is, per its own docs, "a library for React Native, bringing local notification support to both Android & iOS applications." Yet you are taking over and breaking adjacent package functionality BY DEFAULT and without a way to opt-out.

If there is further discussion or justification of this, please share. If Notifee is intended to be the end-all be-all for all types of Notifications, please state this goal, and clarify that in the React Native Firebase documentation (since intvertase manages both). As-is it feels extremely developer-hostile, unexpected, and under-documented across both packages.

In the meantime, for developers, switching back to version 6.x seems to be the only to keep existing remote notification functionality unbroken.

cc https://github.com/invertase/notifee/issues/755 cc https://github.com/invertase/notifee/issues/847

mikehardy commented 10 months ago

I'm inclined to agree with you.

I need to read more of the history of the decision in this library as this change happened while I wasn't so involved with it.

In the past an NSE was almost table-stakes for Notifee because without it you couldn't skin an FCM that had a notification block on iOS, and if you didn't have a notification block in the FCM you were doomed to terrible delivery performance on iOS

That (or something just like that) might be integrally related to the decision or orthogonal - I don't know yet. But since I work on react-native-firebase and notifee at the moment I can try to mop this up even though it is kind of an "in between" problem, since I've got a handle on both sides

dmateertf commented 10 months ago

I hope this isn't off-topic, but I feel like this may be the root issue of a question I opened up in discussions. I'm just trying to get familiar now with these libraries and their interaction, so I may be off-base here, but if someone with more experience could confirm that this is related (and if there is ANY reasonable workaround, haha!), I'd be grateful. In the meantime maybe I'll revert to v6 and see if that works.

lucasbasquerotto commented 10 months ago

Just in case for those that just want to open the app when the notification is tapped (and notifee make it not work when using onNotificationOpenedApp), you can instead do:

import notifee, { Event, EventType } from '@notifee/react-native';

const handlePushEvent = async (event: Event) => {
    switch (event?.type) {
        case EventType.PRESS: {
            // your logic here
            break;
        }
        default:
            break;
    }
}

notifee.onBackgroundEvent(handlePushEvent);

const App = () => {
    React.useEffect(() => {
        const unsubscribe = notifee.onForegroundEvent((event) => {
            void handlePushEvent(event);
        });

        return () => { unsubscribe(); };
    }, []);

    return ...
}
Darren120 commented 9 months ago

Notifee is, per its docs, "a library for React Native, bringing local notification support to both Android & iOS applications."

Yet it purposefully and knowingly breaks React Native Firebase's remote notification tap handlers in v7.x (link):

onNotificationOpenedApp and getInitialNotification from RNFB Messaging will no longer trigger as Notifee will handle the event. Should not require any code changes to these RNFB event handlers, as events on Android will continue to work as normal

onNotificationOpenedApp and getInitialNotification are effectively required to do anything interesting with remote notifications. It's difficult for me to imagine any serious app using remote notifications without using them.

I'm not familiar with the intricacies of this decision, but to me the stated goal ("This allows quick actions from remote notifications to be supported without the need of a NSE") does not nearly justify the change. Handling quick actions is rare, whereas handling simple taps is extremely common (basically table stakes). This change purposefully and knowingly breaks an adjacent library irreparably in exchange for a slightly simpler implementation of itself.

There was also a simple solution requested back in April, here: #755

I would like to see a functionality where I can turn that off in notifee, something like: notifee.setRemoteHandling(false);

I agree with it - except that I feel the more proper solution is to flip it around: notifee should NOT break other libraries by default. It should be opt-in, i.e. something like this:

To handle remote notifications with Notifee, use notifee.setRemoteHandling(true); NOTE: If you use React Native Firebase's Messaging library, by setting remote handling to true, onNotificationOpenedApp and getInitialNotification will no longer trigger.

Again, Notifee is, per its own docs, "a library for React Native, bringing local notification support to both Android & iOS applications." Yet you are taking over and breaking adjacent package functionality BY DEFAULT and without a way to opt-out.

If there is further discussion or justification of this, please share. If Notifee is intended to be the end-all be-all for all types of Notifications, please state this goal, and clarify that in the React Native Firebase documentation (since intvertase manages both). As-is it feels extremely developer-hostile, unexpected, and under-documented across both packages.

In the meantime, for developers, switching back to version 6.x seems to be the only to keep existing remote notification functionality unbroken.

cc #755 cc #847

which 6.x are u using where it still supports remote notifs?

go-sean-go commented 9 months ago

which 6.x are u using where it still supports remote notifs?

@Darren120 There is literally only one: 6.0.0

Specifically, in package.json:

"@notifee/react-native": "^6.0.0",
zirho commented 9 months ago

Just in case for those that just want to open the app when the notification is tapped (and notifee make it not work when using onNotificationOpenedApp), you can instead do:

import notifee, { Event, EventType } from '@notifee/react-native';

const handlePushEvent = async (event: Event) => {
    switch (event?.type) {
        case EventType.PRESS: {
            // your logic here
            break;
        }
        default:
            break;
    }
}

notifee.onBackgroundEvent(handlePushEvent);

const App = () => {
    React.useEffect(() => {
        const unsubscribe = notifee.onForegroundEvent((event) => {
            void handlePushEvent(event);
        });

        return () => { unsubscribe(); };
    }, []);

    return ...
}

Unfortunately, this does not seem to work either on my iOS device with notifee 6 or 7.

Update: my case was this "Background App Refresh" off for all apps in general settings. https://github.com/invertase/react-native-firebase/commit/8f4297659ae34c42b7bf670ca459e964d808570d#diff-9a9312f4c43d06d6871ded72961084bbffb3c57954c9f01a244d51bc3a569042R316-R317

fobos531 commented 9 months ago

I'm inclined to agree with you.

I need to read more of the history of the decision in this library as this change happened while I wasn't so involved with it.

In the past an NSE was almost table-stakes for Notifee because without it you couldn't skin an FCM that had a notification block on iOS, and if you didn't have a notification block in the FCM you were doomed to terrible delivery performance on iOS

That (or something just like that) might be integrally related to the decision or orthogonal - I don't know yet. But since I work on react-native-firebase and notifee at the moment I can try to mop this up even though it is kind of an "in between" problem, since I've got a handle on both sides

Hey @mikehardy !

Just stumbled upon this after recently implementing features like handling basic notification taps (NOT quick actions, just tapping on the notification) and I would like to chime in and say that I fully agree with OP as well. As the OP mentioned, maybe instead of Notifee "taking over" the background notification handling by default it could instead be opt-in. I've found that react-native-firebase is so far completely sufficient for my remote notification support, although I do appreciate all the ways in which Notifee enhances the remote notification support and I welcome it through an opt-in basis.

github-actions[bot] commented 8 months ago

Hello πŸ‘‹, to help manage issues we automatically close stale issues.

This issue has been automatically marked as stale because it has not had activity for quite some time.Has this issue been fixed, or does it still require attention?

This issue will be closed in 15 days if no further activity occurs.

Thank you for your contributions.

dmateertf commented 8 months ago

This issue has been automatically marked as stale because it has not had activity for quite some time.Has this issue been fixed, or does it still require attention?

It is still broken and requires attention.

github-actions[bot] commented 7 months ago

Hello πŸ‘‹, to help manage issues we automatically close stale issues.

This issue has been automatically marked as stale because it has not had activity for quite some time.Has this issue been fixed, or does it still require attention?

This issue will be closed in 15 days if no further activity occurs.

Thank you for your contributions.

fobos531 commented 7 months ago

not stale

Brma1048 commented 7 months ago

Hi, did anyone find a solution @mikehardy @fobos531 @go-sean-go?

My use case is, that if a user presses a notification i would like to navigate to a specific screen based on the notification data. For Android i uses data-only notifications and for iOS i uses remote-notifications. For iOS it works perfect, because there the notifee.onForgroundEvent gets called when the app gets opened from a pushnotification. But for android when the app is in background state only the notifee.onBackgroundEventtriggers. As the docs says we need to call notifee.onBackgroundEvent as soon as possible so i have this call inside my index.ts before I call AppRegistry.registerComponent. But at this point i don't have access to useNavigation() from '@react-navigation' and therefore I can't navigate to a specific screen.

My workaround for now is that i use Deeplinking from react-navigation and call inside notifee.onBackgroundEvent await Linking.openURL(foo://<path to screen>). But for this approach it is a hassle with the params and business code also needs some changes to work with this workaround.

It would be great if onForegroundEvent also gets called on Android when a user clicks on a notification or if onNotificationOpenedApp still works.

darlanhms commented 6 months ago

Hi, did anyone find a solution @mikehardy @fobos531 @go-sean-go?

My use case is, that if a user presses a notification i would like to navigate to a specific screen based on the notification data. For Android i uses data-only notifications and for iOS i uses remote-notifications. For iOS it works perfect, because there the notifee.onForgroundEvent gets called when the app gets opened from a pushnotification. But for android when the app is in background state only the notifee.onBackgroundEventtriggers. As the docs says we need to call notifee.onBackgroundEvent as soon as possible so i have this call inside my index.ts before I call AppRegistry.registerComponent. But at this point i don't have access to useNavigation() from '@react-navigation' and therefore I can't navigate to a specific screen.

My workaround for now is that i use Deeplinking from react-navigation and call inside notifee.onBackgroundEvent await Linking.openURL(foo://<path to screen>). But for this approach it is a hassle with the params and business code also needs some changes to work with this workaround.

It would be great if onForegroundEvent also gets called on Android when a user clicks on a notification or if onNotificationOpenedApp still works.

My use case is pretty much the same and I wish I could opt-in for remote messages functionalities on this lib

go-sean-go commented 6 months ago

Hi, did anyone find a solution @mikehardy @fobos531 @go-sean-go?

Sticking to 6.x, sadly.

github-actions[bot] commented 5 months ago

Hello πŸ‘‹, to help manage issues we automatically close stale issues.

This issue has been automatically marked as stale because it has not had activity for quite some time.Has this issue been fixed, or does it still require attention?

This issue will be closed in 15 days if no further activity occurs.

Thank you for your contributions.

go-sean-go commented 5 months ago

Not stale.

github-actions[bot] commented 4 months ago

Hello πŸ‘‹, to help manage issues we automatically close stale issues.

This issue has been automatically marked as stale because it has not had activity for quite some time.Has this issue been fixed, or does it still require attention?

This issue will be closed in 15 days if no further activity occurs.

Thank you for your contributions.

fobos531 commented 4 months ago

not stale

github-actions[bot] commented 4 months ago

Hello πŸ‘‹, to help manage issues we automatically close stale issues.

This issue has been automatically marked as stale because it has not had activity for quite some time.Has this issue been fixed, or does it still require attention?

This issue will be closed in 15 days if no further activity occurs.

Thank you for your contributions.

dmateertf commented 3 months ago

not stale

valntyn commented 3 months ago

not stale

JoniVR commented 3 months ago

For everyone following this: for me this issue is resolved with #985. I have applied that change using patch-package until a new version is released.

CloudCamilon commented 3 months ago

Bumping this thread as to increase awareness. This library has been great for the most part but the change for messaging.setBackgroundMessageHandler() and messaging.getInitialNotification() has been frustrating. We implemented on press and deep linking events for both iOS and Android harmoniously with RNFB and Notifee ^7.8.0 functions. After a recent regression testing, we noticed background/quit notification events for Android are not working.

Thanks for the fix @JoniVR for iOS though! Hopefully a similar solution is implemented in Android too. πŸ™

vaniyokk commented 2 months ago

@CloudCamilon how do you implement showing notifications from hidden (background, not killed) state for Android? Do you use data-only notifications? For me, notification is delivered, but it's displaying only when I'm opening the app from background, or if app was killed, but never for the app running in background.

markalanevans commented 2 months ago

not stale.

thank goodness for reddit https://www.reddit.com/r/reactnative/comments/17s1o0x/onnotificationopenedapp_not_triggering_on_opening/

And you guys.

CloudCamilon commented 2 months ago

@CloudCamilon how do you implement showing notifications from hidden (background, not killed) state for Android? Do you use data-only notifications? For me, notification is delivered, but it's displaying only when I'm opening the app from background, or if app was killed, but never for the app running in background.

We implemented background handling for Android using the standard onNotificationOpenedApp() and getInitialNotification() by RN Firebase.

Nope, we use the regular notification messages with the optional data payload for our processing needs. This is probably why notifications are not popping up when it is out of your application since notification messages are automatically handled by the sdk/os of RN Firebase and data-only acts as a "silent" notification.

vaniyokk commented 2 months ago

I finally got background notifications + deep linking working by using react-native-push-notification instead of Notifee. It's not updated for 3 years but surprisingly gets job done better.

I receive messages with notification property by our backend and data-only from third-party service and all of them working for all cases (foreground, background, killed), deep links with onNotificationOpenedApp, getInitialNotification are working too! (we're using AppsFlyer for attribution). So, I receive notifications via setBackgroundMessageHandler callback and in case it's data-only notification display them via react-native-push-notification:

PushNotification.localNotification({
      channelId: 'default',
      title: '...',
      message: '...',
      smallIcon: "ic_small_icon",
      playSound: true
    });

I think it's kinda weird that library designed to show local notifications not working for this common case. Hope this will help someone.

I think it should be changed something in Notifee to not break deep link attribution AND to just be able to show notification from background (hidden) state, now it just doesn't displayed in Notifee until app switched to foreground. (see this issue, this is still actual problem https://github.com/invertase/notifee/issues/830)

markalanevans commented 2 months ago

Nice.

CloudCamilon commented 1 month ago

I finally got background notifications + deep linking working by using react-native-push-notification instead of Notifee. It's not updated for 3 years but surprisingly gets job done better.

I receive messages with notification property by our backend and data-only from third-party service and all of them working for all cases (foreground, background, killed), deep links with onNotificationOpenedApp, getInitialNotification are working too! (we're using AppsFlyer for attribution). So, I receive notifications via setBackgroundMessageHandler callback and in case it's data-only notification display them via react-native-push-notification:

PushNotification.localNotification({
      channelId: 'default',
      title: '...',
      message: '...',
      smallIcon: "ic_small_icon",
      playSound: true
    });

I think it's kinda weird that library designed to show local notifications not working for this common case. Hope this will help someone.

I think it should be changed something in Notifee to not break deep link attribution AND to just be able to show notification from background (hidden) state, now it just doesn't displayed in Notifee until app switched to foreground. (see this issue, this is still actual problem #830)

Well, switching to a different library is definitely one solution to consider depending on the severity and resources that you have. I just wish that Invertase resolves this solution soon otherwise we have no choice but to downgrade versions.

github-actions[bot] commented 4 weeks ago

Hello πŸ‘‹, to help manage issues we automatically close stale issues.

This issue has been automatically marked as stale because it has not had activity for quite some time.Has this issue been fixed, or does it still require attention?

This issue will be closed in 15 days if no further activity occurs.

Thank you for your contributions.

mikehardy commented 1 week ago

I believe there is still some thinking to do here

nazmeln commented 1 week ago

So, does the patch make onNotificationOpened work on iOS too? I've just bumped up the lib and sadly it doesnt works on iOS

ceebows commented 1 week ago

notifee still broken for handling foreground on ios