fix(iOS): Allow the original delegate to handle non-Notifee notifications first

gotcha84 commented 9 months ago

Issues

Fixes

925
984

Overview

For notifications received when the app is backgrounded or killed in iOS (through userNotificationCenter:didReceiveNotificationResponse:), the original delegate is not invoked for non-Notifee notifications. The existing code attempts to parse the unknown notification into a format Notifee understands before giving the original delegate a chance to handle it.

I've moved up original delegate so that it handles a non-Notifee notification first (if it exists). If there is no original delegate, then we fall back to parsing the unknown notification and allowing Notifee to handle it.

Testing

Jest tests all pass
iOS end-to-end tests all pass
I have an app that handles both Iterable and Notifee notifications.
1. Before this change: The Iterable notification was being handled by Notifee, resulting in a default behavior of launching to the app home screen.
2. After this change: Notifee falls back to the original delegate when encountering the Iterable notification, resulting in handling the notification correctly & launching the app to the correct screen.

CLAassistant commented 9 months ago

All committers have signed the CLA.

BMR11 commented 9 months ago

@mikehardy, Just wondering if we can get this in as this may be more common case and the bug prevents one listener from other

SMPinCode commented 9 months ago

Tried this. messaging().getInitialNotification() works now when iOS app is terminated.

github-actions[bot] commented 8 months ago

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

fobos531 commented 8 months ago

not stale

@mikehardy Is there a possibility to get this reviewed and merged? It would really help for projects that use libraries that depend on Notifee, but for projects where advanced notifee features are not directly required by the consumer (the developer), instead react-native-firebase is enough. This is exactly the use case I have and would greatly appreciate this change being merged. (same as in android, but that's a story for another PR)

carlbleick commented 8 months ago

If I correctly understand these changes, wouldn't the original delegate always get called instead of the notifee handler? E.g when I use react-native-firebase? Should there be an option to toggle that behaviour?

gezquinndesign commented 8 months ago

Good work @gotcha84 ! Looking forward to seeing this merged.

github-actions[bot] commented 7 months ago

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

jacobmllr95 commented 7 months ago

I can confirm that this solves #913 as well.

I would love to see this merged soon!

github-actions[bot] commented 6 months ago

Hello 👋, this PR has been opened for more than 2 months with no activity on it.

If you think this is a mistake please comment and ping a maintainer to get this merged ASAP! Thanks for contributing!

You have 15 days until this gets closed automatically

fobos531 commented 6 months ago

not stale

balsick commented 6 months ago

is there a plan to merge this PR?

singhayush1403 commented 6 months ago

Can someone please merge this?

helenaford commented 5 months ago

thanks for the fix! 🙏

kiranz commented 5 months ago

Thanks for fixing and merging it. Any idea when this will be included in an upcoming release?

markalanevans commented 4 months ago

@gotcha84 looks like this was merged, but has some linting issues https://github.com/invertase/notifee/actions/runs/9472210287/job/26097204168

@helenaford any reason aside from linting issues to not get another release out with this included?

markalanevans commented 4 months ago

Ok. @gotcha84 i did the cleanup

@helenaford it's clean now.

https://github.com/invertase/notifee/pull/1073

RyuWoong commented 3 months ago

Firstly, thank you, and when do you think it will be deployed?

Roman-ZN commented 3 months ago

@helenaford @mikehardy is it possible to merge the fix #1073 provided by @markalanevans? Currently, the library is incompatible with the latest RN versions since it fails the TS check.

mikehardy commented 2 months ago

Hey there - @notifee/react-native@7.9.0 is published just now with these changes --> https://github.com/invertase/notifee/releases/tag/%40notifee%2Freact-native%407.9.0

invertase / notifee