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

Notifee should not break React Native Firebase #912

Open
go-sean-go opened this issue Nov 17, 2023 · 23 comments
Open

Notifee should not break React Native Firebase #912

go-sean-go opened this issue Nov 17, 2023 · 23 comments

Comments

@go-sean-go
Copy link

go-sean-go commented Nov 17, 2023

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

@mikehardy
Copy link
Contributor

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
Copy link

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
Copy link

lucasbasquerotto commented Dec 5, 2023

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
Copy link

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
Copy link
Author

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
Copy link

zirho commented Dec 30, 2023

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.
invertase/react-native-firebase@8f42976#diff-9a9312f4c43d06d6871ded72961084bbffb3c57954c9f01a244d51bc3a569042R316-R317

@fobos531
Copy link
Contributor

fobos531 commented Jan 2, 2024

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.

Copy link

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
Copy link

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.

Copy link

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
Copy link
Contributor

not stale

@Brma1048
Copy link

Brma1048 commented Mar 6, 2024

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.onBackgroundEvent triggers. 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
Copy link

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.onBackgroundEvent triggers. 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
Copy link
Author

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

Sticking to 6.x, sadly.

Copy link

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
Copy link
Author

Not stale.

Copy link

github-actions bot commented May 9, 2024

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
Copy link
Contributor

fobos531 commented May 9, 2024

not stale

Copy link

github-actions bot commented Jun 6, 2024

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
Copy link

not stale

@valntyn
Copy link

valntyn commented Jun 13, 2024

not stale

@JoniVR
Copy link

JoniVR commented Jun 19, 2024

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
Copy link

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. 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests