-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Run onNotification() in JS, even when the app isn't running at all. #220
Conversation
…ceive on the background listener service.
This should fix: Though as you mention in #170 (comment) , I guess it's arguable that you don't want this going through onNotification (and instead prefer this native-Java processing system which can't be customized at all), for the sake of iOS compatibility? I haven't investigated in detail the options detailed in articles like https://medium.com/posts-from-emmerge/ios-push-notification-background-fetch-demystified-7090358bb66e#.krswqcyl6, but I hold out hope that there's some level of background execution that can work on iOS too, and let library users customize how notifications are processed when the app is not running... |
Thanks for this, I'll take a look at it later. |
@mikelambert I tested your branch but still get no call to onNotification on cold start from notification.
This is the line where the error originates I noticed this was happening when opening the notification and it has something to do with gcm which makes me wonder if this does not work with local notifications ? |
That error looks to be due to the Registration Service, which should be unrelated to what I'm doing (which is processing of notifications), though maybe they interact in strange ways. In the case of my code, you just have to be sure to call PushNotification.configure(...) outside of React.Components, basically called as part of a top-level import, because none of the Components will be initialized in the background thread. Not sure what you're doing exactly, though feel free to link to code/examples of what you're doing and I can take a peek? |
I'm just trying to test this, but need some help. Here's what I've observed so far. My app already receives GCM push notifications when in the background, I don't believe this is a bug. Maybe there's a bug where it doesn't understand messages that arrive in a different format, but that's a separate issue I think. If the app is closed it doesn't receive messages. As far as I can tell from my testing this doesn't address that issue. WRT #170 onNotification only gets fired for push notifications (when the app is either in the foreground or background, but not if it isn't running at all). It does not get fired when the user clicks on an alert in the notification center. And it does not get fired when the app sends a local notification. The only change in behaviour I can see is that when my app sends a local notification then onNotification gets called, but not with the expected local notification, but instead it gets called with the last push notification. So I'm seeing the last push notification over and over again. In summary, I can't merge this change. I don't fully understand what its for or how to test it. Can you please describe exactly what the expected differences in behaviour are please? |
Hey there Nick, sorry for the confusion! Yes, the existing library already delivers notifications to the JS handler when running in the background. But this patch adds support for delivering notifications to the JS handler when the app is closed. I am not sure why it is not working for you in your testing...curious when/how you are calling It does this by loading all the JS from the As to the second bit...the "calls onNotification as the result of a localNotification, but using the last push notification" sounds like a bug. I am not sure what's going on here, but can ask a few questions to narrow it down:
That said, if you have actual code you are running you can point to (runnable?), that might help me diagnose this better. Push Notifications are kind of a hairy thing to handle, especially with all the JS/Native interactions here, so seeing how things get called in your app might help me diagnose what's wrong. Thanks! |
I'm not using scheduled notifications. My |
My question is...what determines when it's "ready to receive notifications". I feel like I'm not able to communicate myself clearly on the special requirements of configure() to receive JS notifications when the app is not running, since you're not clarifying on how your configure is called. Rephrasing, if someone were to Or maybe better, can I see any code? Is it online, or a private codebase? |
Its a private code base. My configure method isn't called as you describe, the app starts up and after a while the |
Correct. The background listener service is loading and running the JS libraries, but it is not initializing any UI components (since there is no Activity created yet, since the user has not opened the app!) In my case, I do this: I call Later, when the UI is initialized (and I have a redux store available), I call |
Thanks for the example. Does your app works on iOS? When I change my app to call |
I haven't enabled notifications yet for my iOS app, so I haven't tested this. But my changes only affected a small bit of JS, so didn't think it was necessary. Again, without code, it is hard to debug your situation. What did you change, exactly? Are you passing an onRegister callback to the first, or second, call to configure()? (Or did you only move your call)? Since I haven't changed any of the iOS code, it basically sounds like you are saying: "There is a bug with the existing react-native-push-notification code on iOS, where it breaks if you call configure too early in the initialization chain" And so not broken by this pull request...but triggered on iOS because of the constraints this new pull request places on app code calling configure()? |
That's correct. I'll look into it more today |
Ah sorry @mikelambert , ignore everything I just said. I was using the iOS simulator. |
Ok, so I've changed my code to call Without adding your change I can get push notifications when the app is backgrounded. But not when its killed. I do see the following in the logs though:
After applying your PR, the change in behaviour when the app is closed is that the notifications are now being delivered to my js code via the So why is In addition, when the app is in the background the notification deliveries get repeated. So the 1st notification received gets delivered once, as expected. The 2nd gets delivered twice, the 3rd 3 times and so on. I backed your changes out and retested agains the current npm version to make sure this wasn't my app being screwy but it behaved as expected. I've looked at the your change, and it seems innocent enough. I certainly can't see why it's repeat delivering notifications. I assume you didn't notice this in your testing? |
Great, glad we're getting closer on this. Yes, your understanding is correct. If the app is not open, a notification will NOT start the app UI. That's impossible to do. What it does do is load the JS, in the background Listener Service. That's why I keep reiterating that any code in UI (including
As far as duplicate notifications, that definitely sounds like a bug. :/ I may have seen something a little similar when operating in hot-reload mode, where (I was guessing) maybe reloads were causing multiple instances of a push handler to be installed. I didn't notice it on a fresh app start/install, or on release mode apps, though. Just to clarify, when you said the second notification gets delivered twice, what do you mean by "delivered"? The |
Exactly, I'm getting repeated calls to |
I think the bug lies in the RNPushNotification constructor, or how it's being called. Inside it calls 3 register methods:
And in turn they add state to the react context. Now this ctor is being called after a push notification is received in
So with each push request that happens when backgrounded, we are adding more and more receivers for RNPushNotificationReceiveNotification. So the react context has increasing number of receivers which explains why I'm seeing more and more repeated calls to Before your change this read
So maybe this is where it's gone wrong? To be honest I'm struggling to follow the code paths and finding it quite confusing. |
…rService as well as the RNPushNotification base code (ie the Activity and JS library). Also prevent popInitialNotification from being run more than once.
Thanks, great debugging, I think that's exactly what's going wrong. (My assumption of hot-reloading was probably unrelated, and I was probably conflating "reload the code to fix a notification bug" with the second notification I sent after reloading.) Anyways, I've just pushed a fix in a separate change, which you can take a look at directly. Basically it factors out the deliver-event-to-JS functionality from RNPushNotification.java, so that it can be used by the ListenerService directly, without the accidental installation of handlers (and without the need for the extra Intent communication that existed before). Hopefully this helps clarify the code a bit, too. |
Ok, great. I've retested and it looks fine - thank you! I'll merge it now. Forgive me for being stupid, but I'm still a little confused about the purpose of this change. If your app isn't running, and doesn't get started when the notification arrives - what's this change for? What can you do if your app isn't running? |
This change, if I understood it correctly, should allow us to add a 'snooze notification' feature to our app. |
In my case, it allows me to:
One could also theoretically use this in a chat application, to track |
Hi @ddobby94 @timhonders @jitenderchand1 @JonesN7 @heminm @vikeri @barroslee This change is in master now. Are you able to try it out before we do a release? (You'll need to change your package.json): From: To: |
|
Modifications works fine for me. Thanks ! |
For me onNotification is only called when I click on the notification. This both works when the app is active and when it is off. When the app is off it starts the app and runs the callback. Was the intention that the callback should run without having to click the notification? |
Yes I forgot about @mikelambert 's point. I had to refactor my app a little to get it to run some js code when it had been killed and a push notification arrived. Basically i needed to invoke the call to |
@npomfret +1 . It's working . You guys have done a great work. Kudos |
I moved my code to index.android.js file. onNotification callback is working fine but my onRegister function stop working. It is working fine when I use the onRegister callback in componentDidmount
|
Hi guys!! @npomfret @mikelambert, I changed my package.json to Also, when app closed -> I received PN -> create Local PN -> when I press on Local PN app opens, but onNotification doesnt invoke... Thanks in advance! |
Seems like onNotification handler doesnt fires when app closed and user click on the local notification. |
@jitenderchand1 Sorry not sure what's up there, did you ever figure it out? I have @bo4a10 So if the app is closed, it is still the responsibility of your app to present the local notification. This should be the same as when the app is open, right? When the app is closed, and the user clicks on the notification, it starts your app with a pending notification. This will not call |
@mikelambert, thanks for response! Yes, you are right, and I expecting the flow like you are described. I called Everything is clear for me except situation when app closed completly, and user will click on local notification that I created after I received remote notification... app just opens, and nothing triggers or 'says' me smth like 'hey, bro, you opened app by clicking on this local push notification'. :) |
I'm not sure. That situation is unrelated to the changes I've made, and you'd have that problem with any notification that triggers your app to open. And yes, My only suggestion would be to dive in and start logging/debugging. In particular, what is returned to |
ok, thanks for help @mikelambert, I'll investigate it deeper. I asked because when PN module in my package.json was in 2.0.1 version - remote notifications appears in status bar with sound and so on (they were visible for end-user), but when I checked out to master version of PN module(for closed app purposes) I can catch remote notification just in Hope that your answer will help me to understand on what point something went wrong, just on clicking on local notifications event or somewhere in global configs of the module. Thanks in advance! |
My personal usage of this library is to process remote notifications, and then produce actual local notifications with all the necessary field. Previously, I believe if you passed all the necessary fields in the remote notification that were necessary for display, then the remote notification would have been presented to the user. (This was never documented here though, I only found it when digging in to the code...) This would have happened for app-loaded or app-closed scenarios, from what I understood. Is this what you observed? (Or did you only see this behavior work for app-loaded scenarios, in which case I'm confused?) But yes, it's possible I broke the old undocumented behavior, since I don't use it.. :/ |
Hi @mikelambert. Seems like I found solution for my issue... I dont think it must be so, but my solution is to set |
Ahh thanks for debugging, though I'm a bit confused as to what's actually not working here. (I'd like to ensure we fix it, if there's a bug...) In my pull request, I had actually split out the code into both With So I'm confused as to what line of code inside the |
Is it related to this recently merged PR #242 ? |
Guys, I'm really sorry! :) Perfect job @mikelambert . Sorry and thank you a lot for help again. :) @npomfret, my 'issue' was not related to #242. I don't use |
@zo0r i think we're ready for a release. |
@npomfret published! Drop me an email to [email protected] if you want npm access too. |
Hi All, I've follow your steps to get the push message when app is completely closed, and when I pressed on notification, the app start and call |
@mikelambert our build started failed when updating from v2.1 to v2.2 of this lib. Turns out this commit breaks it for us. (Note we are building RN from source). Below the the log. Would you have any idea?
|
@chirag04 I am having the same issue. Did you get any fixes? |
I can receive remote notifications fine even from cold start but onNotification isnt called in the background. |
Me neither. Do you still have the issue? |
@jmeiss @robcalcroft did you solve this? Here is my issue, is it the same? https://youtu.be/NwcBsameqQ0 |
I have a question regarding your setup(reduxDispatch) My situation is, so react native app is closed, and when a notification comes in, the notification kicks in, but it can't execute the payload which is navigating thru different UI screens by dispatching some actions.. I noticed that when the app is closed and a notification is received, the UI is not initialized, so those dispatch actions that were sent from onNotification were ignored. So, when the user tries to click the notification, my current behavior is it launches the app and points to the default screen. How can we re dispatch, or reexecute the actions that was received? I tried looking in your repo https://github.com/mikelambert/dancedeets-react but I could'nt find any part of the code calling setup(reduxDispatch) Also, what is the logic or the right way to implement it? I was thinking of storing the dispatch received when the app is closed into some sort of queue, and then when the app launches by clicking the push notification, the queue gets run and anything on it is re-dispatch again. I might sound complicating things, what is your approach for this? |
@vvavepacket Did you solve your issue? |
Make the background listener service start the React JS application (sans Components), and then uses the results of any PushNotification.configure() calls to trigger the JS-based onNotification from the listener service.
This avoids the need to package everything into the Android notification payload to get it to show up, and should hopefully fix a bunch of open issues against this repo.
Note, I haven't explored what's required to get this equivalent functionality working on iOS (and I suspect it may not even be possible at all). But this helps me avoid duplicating functionality on Android and JS by letting me combine it.
I suspect there is additional work that might help combine other native-Android notification processing (RNPushNotificationPublisher and RNPushNotificationBootEventReceiver?) to get them to use JS notification processing. But I don't use them, so they are not in my critical path, and am worried I might break them, since playing with this code is a bit tricky to get right (with threads and the many ways Android notifications can be delivered, etc).