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

addNotificationResponseReceivedListener and useLastNotificationResponse not triggered for killed iOS apps SDK 40 #11343

Closed
maurohmartinez opened this issue Dec 14, 2020 · 18 comments · Fixed by #11382

Comments

@maurohmartinez
Copy link

🐛 Bug Report

After updating to SDK 40 expo-notifications does not trigger addNotificationResponseReceivedListener after app was killed on iOS. This last upgrade did fix the issue on Android, but now I am having that same problem on iOS.

SDK 40
Standalone App (I built a new bundle after updating to SDK 40)
Only on iOS.

Expo CLI 4.0.13 environment info:
System:
OS: macOS 11.0.1
Shell: 5.8 - /bin/zsh
Binaries:
Node: 12.18.4 - /usr/local/bin/node
npm: 6.14.9 - /usr/local/bin/npm
SDKs:
iOS SDK:
Platforms: iOS 14.2, DriverKit 20.0, macOS 11.0, tvOS 14.2, watchOS 7.1
IDEs:
Android Studio: 4.1 AI-201.8743.12.41.6953283
Xcode: 12.2/12B45b - /usr/bin/xcodebuild
npmPackages:
expo: ^40.0.0 => 40.0.0
react: 16.13.1 => 16.13.1
react-dom: 16.13.1 => 16.13.1
react-native: https://github.com/expo/react-native/archive/sdk-40.0.0.tar.gz => 0.63.2
react-native-web: ~0.13.12 => 0.13.18
npmGlobalPackages:
expo-cli: 4.0.13
Expo Workflow: managed

Reproducible Demo

class HandlePushNotifications extends Component {
    componentDidMount() {
        this.onResponseReceivedListener = Notifications.addNotificationResponseReceivedListener(
            this.handlePush <-- This is never triggered when the app was killed on iOS.
        );
    }
}

Steps to Reproduce
Kill the app, Send a push notification, tap on that push (it opens the app)

Expected Behavior vs Actual Behavior
Expected Behavior: Notifications.addNotificationResponseReceivedListener triggers since the user interacted with the push.
Actual Behavior: The triggering of Notifications.addNotificationResponseReceivedListener never occurs.

@maurohmartinez maurohmartinez added the needs validation Issue needs to be validated label Dec 14, 2020
@cruzach
Copy link
Contributor

cruzach commented Dec 14, 2020

Thanks for the bug report! We can look into this. I confirmed the behavior

As a workaround, can you try using the useLastNotificationResponse hook?

@cruzach cruzach added bug and removed needs validation Issue needs to be validated labels Dec 14, 2020
@cruzach cruzach changed the title Problems with expo-notifications after updating to SDK 40 on iOS addNotificationResponseReceivedListener not triggered for killed iOS apps SDK 40 Dec 14, 2020
@maurohmartinez
Copy link
Author

Thanks for the bug report! We can look into this. I confirmed the behavior, but was able to get the correct behavior with useLastNotificationResponse

As a workaround, can you try using the useLastNotificationResponse hook?

I just tried it, but it doesn't trigger with the app killed.
Actually, I have initially created one function using only useLastNotificationResponse for both Android and iOS. After checking that it only worked for Android, I went back to addNotificationResponseReceivedListener for iOS since that worked on SDK 39. And that's when I realised that it no longer works :)

So, I would love to see useLastNotificationResponse working for both Platforms, which means less coded needed :)

@cruzach cruzach changed the title addNotificationResponseReceivedListener not triggered for killed iOS apps SDK 40 addNotificationResponseReceivedListener and useLastNotificationResponse not triggered for killed iOS apps SDK 40 Dec 14, 2020
@hardcodet
Copy link

hardcodet commented Dec 15, 2020

I am observing this even with a backgrounded app on iOS, so the app doesn't even need to be killed. This basically renders my app useless :(

(only tested with one device - in the past, my mileage with push notification varied quite a bit across devices, unfortunately)

@cruzach
Copy link
Contributor

cruzach commented Dec 15, 2020

@hardcodet that's not the behavior I've seen so I suggest you double-check your JS code, and test a minimal example like the one from the docs https://docs.expo.io/versions/latest/sdk/notifications/

@sjchmiela
Copy link
Contributor

Indeed, SDK40 introduced a change to behavior of notifications emitter on iOS which sounds like it could cause what you're describing. So:

  • EXNotificationCenterDelegate which receives all the notification-related events from iOS's UNUserNotificationCenter did and still is remembering notification responses to deliver to its subscribers (a subscriber would be a native module that forwards the event to JS). This let us ensure all the responses get delivered and none is missed (if the delegate didn't remember pending responses it could receive them too early and they would be lost).
  • Up until SDK40 EXNotificationsEmitter which is responsible for subscribing to EXNotificationCenterDelegate and delivering events to JS was subscribing to it only once JS subscribed to it (to put it the other way only once JS successfully subscribed to EXNotificationsEmitter did it subscribe to EXNotificationCenterDelegate which was causing the delegate to forward all pending responses to it). This however did not work well for some race conditions reasons and some notification responses were still getting lost. To fix that…
  • since SDK40 EXNotificationsEmitter subscribes to EXNotificationCenterDelegate as soon as it is initialized. This causes any pending notification responses to be delivered to it earlier (for example even before JS subscribes to the emitter), so if we didn't do anything special we would cause many responses to be missed. However we did guard against this situation by holding a reference to last notification response. This is what useLastNotificationResponse hook uses to fetch initial value — it fetches last notification response received by the app.
  • frankly speaking I don't see any reason now why EXNotificationsEmitter has to subscribe as soon as it is initialized. I think this may be a relict of a previous implementation of this hook that required global always-running listener. I'll test the alternative and get back to you.

All in all what I wanted to say is:

  • the addNotificationResponseReceivedListener not being called when the app is being started (from kill) is a regression, but
  • useLastNotificationResponse should always let you fetch proper value as it uses an alternative way to fetch notification response received last,
  • if useLastNotificationResponse doesn't work for you this is something we should fix in the first place, it is expected to always work
  • as a workaround you can try using getLastNotificationResponseAsync method yourself:
    import NotificationsEmitterModule from 'expo-notifications/build/NotificationsEmitterModule';
    
    const response = await NotificationsEmitterModule.getLastNotificationResponseAsync();

I just wrote this snack that shows how one could use/verify if useLastNotificationResponse works. Let me know if you encounter any more issues with it — https://snack.expo.io/txn0fymwV. I've also recorded a short video showing how do I test the hook — https://www.dropbox.com/s/h6oe2jqi2pqz5o3/RPReplay_Final1608044876.MP4?dl=0

@hardcodet
Copy link

hardcodet commented Dec 15, 2020

@cruzach As said, my mileage does vary. My push notification code hasn't changed in months, and some devices seem to work, others not, and some used to work and no longer do (or no longer do for now). An sometimes, those notifications just come in 10-20 mins too late (as observed yesterday on my Android phone). Been like that for a while though. I'm not convinced this part of expo is production-ready TBH.

@maurohmartinez
Copy link
Author

maurohmartinez commented Dec 15, 2020

@sjchmiela For me the problem is that useLastNotificationResponse does not trigger with the app killed on iOS. My impression is that on your example the app is backgrounded, which doesn't present issue on my apps.

@georgeMorales
Copy link

I have the same problem as @maurohmartinez, it works on iOS when the application is in the background but not when it is kill

@sjchmiela
Copy link
Contributor

sjchmiela commented Dec 15, 2020

Hey guys, I just reproduced it, I'll take a look into what we can do to fix this. As a workaround I can only suggest ejecting to bare workflow where this bug does not exist as far as I know.

EDIT: the problem is most probably caused by ExpoKit registering its own legacy notification manager before aforementioned EXNotificationsEmitter is initialized which causes the legacy manager to consume all pending notification responses — they never get to expo-notifications.

We'll fix this as soon as possible.

@sjchmiela
Copy link
Contributor

Actually, since it's ExpoKit (i.e. legacy notifications) which consumes the notification before expo-notifications, as a hacky workaround one could actually leverage this and access the initial notification response, i.e. the one that should be received when it opens the app from killed state, from ExpoKit infrastructure, so initialProps!

If the app is being started as a result of tapping on a notification and useLastNotificationResponse does not return any value you could use props.notification from properties passed to top-level App component to access some of the information expo-notifications would provide you that is documented here (courtesy of Legacy Notifications).

Doing it like this:

export default function App(initialProps: any) {
  return (
    /* doesn't matter */
        <Text style={styles.text}>
          initialProps: {JSON.stringify(initialProps, null, 2)}
        </Text>
  );
}

I got:

I think the actual proper fix shouldn't take too long to land (I would expect it to be available before the end of this week), but that's some workaround you could use if you need.

@imhazige
Copy link

I am waiting this fix. I am using hook useLastNotificationResponse with sdk40. when APP killed, Android works but ios 12 does not works.

sjchmiela added a commit that referenced this issue Dec 16, 2020
…nly once JS subscribes to it (#11382)

# Why

Fixes a bug introduced in #10883 which causes initial notification responses to get lost in standalone iOS apps.

Along with #11378 I believe it fixes #11343 and may also finally fix #9866.

# How

Using native breakpoints I've noticed `EXNotificationCenterDelegate`'s `addDelegate:` is called more times than I would expect it to.

It turned out before `EXScopedNotificationsEmitter` was subscribing to `EXNotificationCenterDelegate` (it would then receive pending notification response) an unscoped `EXNotificationsEmitter` was subscribing and consuming pending responses. Since unscoped emitter wasn't being used and exposed to the app later, it consumed the response into the void.

Most probably unimodules have a bug where instances of overridden modules are still kept in memory (since `setModuleRegistry:` is called on them). Fixing this bug is out of the scope of this pull request.

These changes are based on how it was implemented before dfda625. I can't see any reason why `EXNotificationsEmitter` would need to register for notifications as soon as module registry is ready, so let's roll back to what we know worked well.

# Test Plan

I have verified in an ExpoKit workspace that when the app is opened from a notification, the notification response is delivered to `useLastNotificationResponse`:

https://www.loom.com/share/586a906c538f47f0940b2cc83582fb29
sjchmiela added a commit that referenced this issue Dec 16, 2020
…livered unless they're delivered to expo-notifications (#11378)

# Why

It's a nice counterpart to #11382 which ensures #11343 and #9866 do not happen again.

# How

Follow-up to #9478. There are two places where the notification response can be "consumed" — when it is received (covered by #9478) and when it is delivered to a new delegate (covered by this PR).

# Test Plan

I have verified in #11382 that workspace with this and #11382 changes delivers initial notification response to the app.
sjchmiela added a commit that referenced this issue Dec 16, 2020
…nly once JS subscribes to it (#11382)

# Why

Fixes a bug introduced in #10883 which causes initial notification responses to get lost in standalone iOS apps.

Along with #11378 I believe it fixes #11343 and may also finally fix #9866.

# How

Using native breakpoints I've noticed `EXNotificationCenterDelegate`'s `addDelegate:` is called more times than I would expect it to.

It turned out before `EXScopedNotificationsEmitter` was subscribing to `EXNotificationCenterDelegate` (it would then receive pending notification response) an unscoped `EXNotificationsEmitter` was subscribing and consuming pending responses. Since unscoped emitter wasn't being used and exposed to the app later, it consumed the response into the void.

Most probably unimodules have a bug where instances of overridden modules are still kept in memory (since `setModuleRegistry:` is called on them). Fixing this bug is out of the scope of this pull request.

These changes are based on how it was implemented before dfda625. I can't see any reason why `EXNotificationsEmitter` would need to register for notifications as soon as module registry is ready, so let's roll back to what we know worked well.

# Test Plan

I have verified in an ExpoKit workspace that when the app is opened from a notification, the notification response is delivered to `useLastNotificationResponse`:

https://www.loom.com/share/586a906c538f47f0940b2cc83582fb29
sjchmiela added a commit that referenced this issue Dec 16, 2020
…livered unless they're delivered to expo-notifications (#11378)

# Why

It's a nice counterpart to #11382 which ensures #11343 and #9866 do not happen again.

# How

Follow-up to #9478. There are two places where the notification response can be "consumed" — when it is received (covered by #9478) and when it is delivered to a new delegate (covered by this PR).

# Test Plan

I have verified in #11382 that workspace with this and #11382 changes delivers initial notification response to the app.
@sjchmiela
Copy link
Contributor

Copying from #9866 (comment):

Issue has been closed for fixes have been merged, please allow up to 24 hours for them to be available when you run expo build:ios (it's pretty late where I am and although I believe there will be no problems when we deploy to production I want to be a responsible citizen and not deploy and go to sleep).

As a foretaste and to some degree proof let me share with you this video — https://www.dropbox.com/s/u2ih7umzt1zbxbp/RPReplay_Final1608160452.mp4?dl=0 🙂

@sjchmiela
Copy link
Contributor

Copying #9866 (comment):

Fix has been deployed to production Turtle builders! 🎉 All iOS apps built after December 17, 2020, 15:45 UTC will have fixes included and should not longer suffer from this problem.

This being said, let us know if this or similar problem ever rises again!

This being said, let us know if this or similar problem ever rises again! 😃

@maurohmartinez
Copy link
Author

Copying #9866 (comment):

Fix has been deployed to production Turtle builders! 🎉 All iOS apps built after December 17, 2020, 15:45 UTC will have fixes included and should not longer suffer from this problem.
This being said, let us know if this or similar problem ever rises again!

This being said, let us know if this or similar problem ever rises again! 😃

Yeahhhhh... It works!!!! Both, Android and iOS. You made my day!

@imhazige
Copy link

Copying #9866 (comment):

Fix has been deployed to production Turtle builders! 🎉 All iOS apps built after December 17, 2020, 15:45 UTC will have fixes included and should not longer suffer from this problem.
This being said, let us know if this or similar problem ever rises again!

This being said, let us know if this or similar problem ever rises again! 😃

@sjchmiela, Good job! need any npm stuff on local dev?

@sjchmiela
Copy link
Contributor

If by any npm stuff on local dev you mean:

  • any patches made to packages on local dev — no, the fixes were native-only so if you're in managed workflow you don't need to worry, I updated the native code for you!
  • any package updates — no, the fixes were native-only, so:
    • if you're in managed workflow you don't need to worry, I updated the native code for you!
    • if you're in bare workflow you don't need to worry, the bug did not exist there (it was caused by stuff we do in standalone apps and Expo Client). 🙂

@yoojeen126
Copy link

yoojeen126 commented Dec 31, 2020

Same issue. How can i do it in class component?
I temporary used @sjchmiela 's comment source NotificationsEmitterModule.getLastNotificationResponseAsync
This code seems working fine with android and ios. I hope fix this issue next version.

@github-actions
Copy link
Contributor

Some changes in the following packages that may fix this issue have just been published to npm under next tag 🚀

📦 Package 🔢 Version ↖️ Pull requests 📝 Release notes
expo-notifications 0.9.0 #11382 CHANGELOG.md

If you're using bare workflow you can upgrade them right away. We kindly ask you for some feedback—even if it works 🙏

They will become available in managed workflow with the next SDK release 👀

Happy Coding! 🎉

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants