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

[unimodules][android][notifications] Event listener can miss emit() calls made soon after app launch #9866

Closed
cruzach opened this issue Aug 20, 2020 · 101 comments · Fixed by #11382

Comments

@cruzach
Copy link
Contributor

cruzach commented Aug 20, 2020

🐛 Bug Report

Summary of Issue

If you call Notifications.addResponseReceivedListener in useEffect for example, the listener will not be triggered when opening an app from a completely killed state via a notification. This can also be demonstrated by adding a timeout to componentDidMount as shown here. Without 1 second timeout- events come through; with 1 second timeout- event's do not come through.

I should note that the emit method is getting called in this case.

Environment - output of expo diagnostics & the platform(s) you're targeting

Reproducible Demo

https://snack.expo.io/@charliecruzan/push-notifications

Steps to Reproduce

Need to run the app as a published experience or standalone app (or bare debug build) to see the behavior. Press "show notification" and kill the app, tap the notification, observe that response listener isn't triggered.

Workaround

The workaround (and possibly long-term solution) is to add the listener outside of any component, then it works as expected (presumably bc of how much sooner it's getting called?). I changed the docs to suggest this so fewer people run into this issue

@cruzach cruzach added needs validation Issue needs to be validated Android Notifications and removed needs validation Issue needs to be validated labels Aug 20, 2020
@cruzach cruzach changed the title [unimodules][notifications] Event listener can miss emit() calls made soon after app launch [unimodules][android][notifications] Event listener can miss emit() calls made soon after app launch Aug 20, 2020
@byCedric byCedric added the bug label Aug 21, 2020
@dmitri-wm

This comment has been minimized.

@cruzach

This comment has been minimized.

@Aryk

This comment has been minimized.

@cruzach

This comment has been minimized.

@giautm

This comment has been minimized.

@Aryk

This comment has been minimized.

@cruzach
Copy link
Contributor Author

cruzach commented Aug 24, 2020

exp.notification is not documented and not explicitly supported, so relying on that feature isn't recommended

@Aryk
Copy link

Aryk commented Aug 24, 2020

exp.notification is not documented and not explicitly supported, so relying on that feature isn't recommended

Got it, so for ios is there any work around at the moment (besides ejecting)? I suppose checking for exp.notification and using that is better than nothing.

@cruzach
Copy link
Contributor Author

cruzach commented Aug 24, 2020

Using the legacy notifications API listener works, as well. Other than that there's no workaround besides waiting for SDK 39 🙁 sorry that you pretty much just have to wait

@dmitri-wm
Copy link

If you're testing this while running locally in the Expo client, e.g. with expo start, notification responses don't come through when the app is killed. You should build your app with expo build:android

Thank you for reply, yes this works. Just to clarify - to test any changes related to handling notifications when app is killed I have 2 options 1) eject from expo and test locally 2) create expo build. No other options, correct?

Thanks again for your help

@cruzach
Copy link
Contributor Author

cruzach commented Aug 25, 2020

You can also run expo publish and test in the Expo client app through the published project

@arveenkumar55

This comment has been minimized.

@xLesterGG

This comment has been minimized.

@cruzach
Copy link
Contributor Author

cruzach commented Aug 26, 2020

I opened this new issue to try and keep the conversation focused around the root issue, and this thread is turning into troubleshooting one of the resulting problems, which I've already shared the answer for in the original issue- #6943. Any comments posted here that are answered in the other issue will be marked as off-topic (sorry to do this, but too many off-topic comments makes discussing the original issue I posted much more difficult)

@arveenkumar55 please refer to this comment

@xLesterGG as far as I know there's no problem running Linking.openUrl from addResponseReceivedListener. However, maybe it's easier for you to follow this react navigation guide on navigating outside of your component, rather than using Linking.openUrl

@ozerty
Copy link

ozerty commented Oct 1, 2020

Hi @cruzach,
Works when app is in background, but not when the app is killed.
The issue still persist on Android with both SDK 38 and SDK 39 in a managed workflow.
I try to move out addNotificationResponseReceivedListener outside of my component as suggested, but it doesn't work.

Thanks

@JamieS1211
Copy link
Contributor

It is noted in the React documentation that useEffect is called after paint and useLayoutEffect is called 'synchronously after all DOM mutations' hence 'useLayoutEffect fires in the same phase as componentDidMount and componentDidUpdate'.

https://reactjs.org/docs/hooks-reference.html#timing-of-effects
https://reactjs.org/docs/hooks-reference.html#uselayouteffect

As such I have tested useLayoutEffect and still had no success.

Further to that, my specific implementation to navigate on notification was rather inconvenient when being placed outside the navigation container. For any others who have this issue and use ReactNavigation, my specific workaround here was to make a navigation container ref with a promise version, allowing await for the navigation to be mounted before completing the notification handling.

Setup the ref for the navigation container (ignore the typescript typings if relevant), the navigation ref promise only resolves after items have been mounted inside the navigation container (this is reason for checking "getRootState())".

import { createRef } from "react"

import { NavigationContainerRef } from "@react-navigation/core"

const navigationRef = createRef<NavigationContainerRef>()

export const navigationRefPromise: Promise<typeof navigationRef> = new Promise((resolve) => {
    const checkValue = () => {
        if (navigationRef.current && navigationRef.current.getRootState()) {
            return resolve(navigationRef)
        }

        setTimeout(checkValue, 50)
    }

    checkValue()
})

export default navigationRef

When processing the notification, if navigation data is present and results in a valid navigation action (based on navigation linking config), complete the navigation dispatch after the navigation ref promise has resolved.

addNotificationResponseReceivedListener((notificationResponse) => {
    const notificationData = notificationResponse.notification.request.content.data

    if (hasKeyGuard(notificationData, "navigationLink") && typeof notificationData.navigationLink === "string") {
        try {
            const state = getStateFromPath(notificationData.navigationLink, linking.config)

            if (state) {
                const action = getActionFromState(state)

                navigationRefPromise.then((navRef) => {
                    if (action && navRef.current) {
                        navRef.current.dispatch(action)
                    }
                })
            }
        } catch (error) {
            alert(error)
        }
    }
})

@KlasRobin
Copy link

KlasRobin commented Oct 13, 2020

Hi @cruzach,
Works when app is in background, but not when the app is killed.
The issue still persist on Android with both SDK 38 and SDK 39 in a managed workflow.
I try to move out addNotificationResponseReceivedListener outside of my component as suggested, but it doesn't work.

Thanks

Same problem here. Have also tried moving addNotificationResponseReceivedListener outside of App.js with no luck. Background and foreground works fine. Edit: Running SDK 39 in managed workflow.

@antwaven
Copy link

Hi @cruzach,
Works when app is in background, but not when the app is killed.
The issue still persist on Android with both SDK 38 and SDK 39 in a managed workflow.
I try to move out addNotificationResponseReceivedListener outside of my component as suggested, but it doesn't work.
Thanks

Same problem here. Have also tried moving addNotificationResponseReceivedListener outside of App.js with no luck. Background and foreground works fine. Edit: Running SDK 39 in managed workflow.

Hi, I too have the exact same issue. SDK 39 managed workflow. Tried it on standalone Android and iOS, inside useEffect or in the same place as Notifications.setNotificationHandler with no luck. If I can provide any other information to help find this issue please let me know.

@coopbri
Copy link

coopbri commented Oct 14, 2020

Confirming same problem as @KlasRobin, @antwaven, and @ozerty. Background/foreground work fine, just not when app is killed/not running. Also managed workflow, SDK 39.

@coopbri
Copy link

coopbri commented Dec 16, 2020

Hi @imhazige, I had issues on iOS 14, and still on Android for some reason (I think it had to do with the navigation mounting or something). I know this is not an ideal solution, but I switched to OneSignal for our app and it's been working great on both platforms (minus the typical FCM/APNs pitfalls, particularly slightly delayed notifications). This may be considered off-topic for the thread, but wanted to mention my success story just to indicate it as an option for you. Keep in mind it is a library with native bindings, so the Expo bare workflow is required if you choose to go that route.

I have no doubt the Expo notifications API will continue to improve over time, but I did not have time to wait, unfortunately.

@imhazige
Copy link

Hi @imhazige, I had issues on iOS 14, and still on Android for some reason (I think it had to do with the navigation mounting or something). I know this is not an ideal solution, but I switched to OneSignal for our app and it's been working great on both platforms (minus the typical FCM/APNs pitfalls, particularly slightly delayed notifications). This may be considered off-topic for the thread, but wanted to mention my success story just to indicate it as an option for you. Keep in mind it is a library with native bindings, so the Expo bare workflow is required if you choose to go that route.

I have no doubt the Expo notifications API will continue to improve over time, but I did not have time to wait, unfortunately.

@coopbri

Thank you for the response.

I am using SDK40 and it works on android, for ios it seems to have this issue (#11343) need to be fixed.

I have no choice for now as I have to bind to expo managed flow. Onesignal is better choice if I can use bareflow.

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

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

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!

@Aryk
Copy link

Aryk commented Dec 27, 2020

@sjchmiela - Just upgraded to SDK 40

On my Galaxy S8, standalone app, managed, with Android Version 9 (I cannot upgrade the OS bc phone is not supported), the problem still persists.

See above: #9866 (comment)

Yes, I've also tried useLastNotificationResponse, but as I suspected in my previous comment, because it relies on the same hook, it still didn't work.

I've tried to make a minimal repro case but seems I can't reproduce it without the complexity of my app.

Should I make a new issue for this?

@Aryk
Copy link

Aryk commented Dec 27, 2020

Can anyone tell me if they can do:

  1. Open notification from dead start on Android version 9.
  2. Have background locations already running on Always via hasStartedLocationUpdatesAsync when the app is being opened.

While background location updates are on, the notifications are not being received from a dead start. When I turned it off, it works.

@alexandrius
Copy link
Contributor

Can anyone tell me if they can do:

  1. Open notification from dead start on Android version 9.
  2. Have background locations already running on Always via hasStartedLocationUpdatesAsync when the app is being opened.

While background location updates are on, the notifications are not being received from a dead start. When I turned it off, it works.

For me notifications aren't wotking even without location updates. I still use #9866 (comment) this workaround

@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! 🎉

@imhazige
Copy link

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!

Just to confirm, this works for me. on ios and android. have not see problem yet.
I am using SDK 40.

@webuijorgeGL
Copy link

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!

Just to confirm, this works for me. on ios and android. have not see problem yet.
I am using SDK 40.

But do you received a warning expected expo-notification V 0.8.2?

@webuijorgeGL
Copy link

webuijorgeGL commented Jan 20, 2021

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! 🎉

will work with the expo SDK 40?

@brentvatne
Copy link
Member

brentvatne commented Feb 3, 2021

@webuijorgeGL - #11382 is in sdk 40, if that's what you're looking for. you cannot, however, use expo-notifications 0.9.0 in sdk 40 - that includes other changes (see the changelog).

https://github.com/expo/expo/blob/sdk-40/packages/expo/bundledNativeModules.json#L103

@MrIceman
Copy link

MrIceman commented Sep 6, 2021

Still doesn't work with latest SDK

@Fuel0
Copy link

Fuel0 commented Sep 7, 2021

Any news ?
In my case addNotificationResponseReceivedListener is not working on background and killed app. I am using Expo SDK 42

@alexandrius
Copy link
Contributor

@MrIceman @Guilhemfuel Just use the useLastNotificationResponse hook

@Fuel0
Copy link

Fuel0 commented Oct 27, 2021

@MrIceman @Guilhemfuel Just use the useLastNotificationResponse hook

It doesn't work either on background and killed. The response will be null...

@federicoromandemo
Copy link

Have you found a workaround for this issue? useLastNotificationResponse didn't work for me either.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet