facebook / react-native

A framework for building native applications using React
https://reactnative.dev
MIT License
117.04k stars 24.07k forks source link

[PushNotificationIOS] No register event ever gets triggered #1613

Closed timminkov closed 8 years ago

timminkov commented 9 years ago

I'm trying to get push notifications working on my device:

Pulling this commit on React 0.5.0.

Adding a PushNotificationIOS.requestPermissions() in my render pops open the permissions box, but when I add:

PushNotificationIOS.addEventListener('register', function(data) {
  console.log(data);
});

I get nothing logged. I've tried in the simulator and on an actual device but once I click the allow button, nothing. I've tried using this component too.

I also tried adding the following code to my AppDelegate.m from this thread:

- (void)application:(UIApplication *)application didRegisterForRemoteNotificationsWithDeviceToken:(NSData *)deviceToken {
  [RCTPushNotificationManager application:application didRegisterForRemoteNotificationsWithDeviceToken:deviceToken];
}

Any ideas?

DannyvanderJagt commented 9 years ago

I just tried to do the same thing, but I didn't get it to work either.

I just wanted the device token so I could use it for the push notifications and my node server. I have found a working solution but it is without the register event. In #1019 the requestPermissions uses a callback for giving you the device token. I have used https://github.com/lazaronixon/react-native/commit/af08fde42e4fb57bb04b563756ac92e4d285ae85 and got it working. (It's only implemented in the SampleApp)

The commit didn't work right away but when I copied the AppDelegate+notification.m and the AppDelegate+notification.h from SampleApp/IOS to Libraries/PushNotificationIOS and added the PushNotificationIOS Library I got it working. (I am not sure that this is a right way of fixing it, I just started to play with IOS and Objective-C yesterday)

I hope it helped and looking forward for a proper fix.

timminkov commented 9 years ago

I tried copying over the two notification files to my PushNotificationIOS directory and adding it to my project, but still no dice.

DannyvanderJagt commented 9 years ago

I'm sorry for my stupid answer yesterday. I didn't read properly and I just started with react and IOS two days ago. However I did some more digging and I think I have found a proper solution.

The problem is that didRegisterForRemoteNotificationsWithDeviceToken in the file RCTPushNotificationManager never gets called. I downloaded the latest version of react-native from the master and as suggested in https://github.com/facebook/react-native/pull/1304 I added this to the bottom of the AppDelegate.m file:

- (void)application:(UIApplication *)application didRegisterForRemoteNotificationsWithDeviceToken:(NSData *)deviceToken {
  [RCTPushNotificationManager application:application didRegisterForRemoteNotificationsWithDeviceToken:deviceToken];
}

And this line to the top of AppDelegate.m:

#import "../../../Libraries/PushNotificationIOS/RCTPushNotificationManager.h"

But I also added this line to AppDelegate.h:

- (void)application:(UIApplication *)application didRegisterForRemoteNotificationsWithDeviceToken:(NSData *)deviceToken;

When I added those line to my code the didRegisterForRemoteNotificationsWithDeviceToken did get called. But it then this error showed up: registerForRemoteNotificationTypes: is not supported in iOS 8.0 and later. RCTPushNotificationManager.m

To fix this I changed this in the RCTPushNotificationManager.m file on line 140:

#if __IPHONE_OS_VERSION_MIN_REQUIRED >= __IPHONE_8_0

into:

#ifdef __IPHONE_OS_VERSION_MIN_REQUIRED >= __IPHONE_8_0

The register event in javascript is now working here and I get the device token as an argument. However I discovered that there is no error handling in the PushNotifications Library, all the errors are going away without notice. I think this should be fixed. Do make sure that all your certificates are valid etc.

I hope this is helped.

timminkov commented 9 years ago

@DannyvanderJagt were you able to get the event to trigger in the simulator?

DannyvanderJagt commented 9 years ago

@timminkov The register event can't be fired in the simulator because remote notifications are not supported in the simulator. Remote/Push notifications can only be used on an actual device. The error event from my fork does fire on the simulator and if your run the app in the simulator it will give you this error: remote notifications are not supported in the simulator which is an actual IOS (Objective-C) error.

timminkov commented 9 years ago

Unfortunately following your instructions (and your commit) I'm STILL unable to even get the remote notifications are not supported in the simulator error message.

marcshilling commented 9 years ago

I got this working by doing the following:

  1. Go to Build Settings in Xcode and under "Header Search Paths" add $(SRCROOT)/node_modules/react-native/Libraries/**
  2. At the top of AppDelegate: #import "RCTPushNotificationManager.h"
  3. Also in AppDelegate, add:
- (void)application:(UIApplication *)application didRegisterForRemoteNotificationsWithDeviceToken:(NSData *)deviceToken
{
  [RCTPushNotificationManager application:application didRegisterForRemoteNotificationsWithDeviceToken:deviceToken];
}
  1. In RCTPushNotificationManager, around line 141, comment out the #if #else #endif and replace it with:
  if ([[UIApplication sharedApplication] respondsToSelector:@selector(registerUserNotificationSettings:)]) { // iOS 8
    id notificationSettings = [UIUserNotificationSettings settingsForTypes:types categories:nil];
    [[UIApplication sharedApplication] registerUserNotificationSettings:notificationSettings];
    [[UIApplication sharedApplication] registerForRemoteNotifications];
  }
  else {
    [[UIApplication sharedApplication] registerForRemoteNotificationTypes:types];
  }

The #if #else macro is checking the minimum supported version for your project. This is a bad check (obviously), because if you are supporting iOS 7 but running on iOS 8, it will call the wrong code. respondsToSelector is the correct way to handle this.

After these steps, I finally got the push notifications popup when calling PushNotificationIOS.requestPermissions();

DannyvanderJagt commented 9 years ago

Looks good and it is working here! A few days ago I fixed this locally but your way is cleaner. To make your code work on the SampleApp I needed to change the "Header Search Paths" to $(SRCROOT)/../../Libraries (recursive)

How are we going to implement this in the main react branch? Because before this fix we could just add the RCTPushNotificationManager to xcode without adding code to the AppDelegate. Is this something that just needs to be added to the docs or is there another way/better way of solving this bug?

Also I do think that the register event, which gives you the device token, and the error event should be implemented. Should this be in the same pull request/issue or should this be separate? (I just started to learn how to contribute to big public repositories.)

DannyvanderJagt commented 9 years ago

I created a new fork of the react repo and added the changes from @marcshilling. I also added in another commit the error event. (Let me know if I should create a new issue for this)

The error event can be used just like this:

PushNotificationIOS.addEventListener('error', function(error){
  console.log(error); // {"NSLocalizedDescription":"no valid 'aps-environment' entitlement string found for application"}
});

At this point the event gives you the original IOS error, which exists of a error type and the message. So the output at this point in javascript can be something like: {"NSLocalizedDescription":"no valid 'aps-environment' entitlement string found for application"}

Should I leave it this way or is it better to separate the key and the error message? This way the key and the message are separated. Personally I prefer this.

PushNotificationIOS.addEventListener('error', function(key, error){
  console.log(key, error); // NSLocalizedDescription, no valid 'aps-environment' entitlement string found for application
});
dorthwein commented 9 years ago

I tried to implement the above fixes using 0.6.0 with no luck. Can any one confirm that y'all are able to get the device token?

Basically the token event still isn't firing it seems.

Thanks

~DFO~

DannyvanderJagt commented 9 years ago

@dorthwein I just downloaded the new 0.6.0 release and implemented the fix. It is working here and the register event gives the device token as a param. Note: This event will only be fired on an actual device and not in a simulator also all the certificates should be available and correct.

I will update (in a few hours) the 0.6-stable branch on my fork with this fix and the the error event I created. This way you will get an error message with the actual IOS error, maybe it helps us the figure this out.

DannyvanderJagt commented 9 years ago

I have updated my fork on my fork. The register event and the error event are both working. The fixes are only implemented in examples/SampleApp for now!

In javascript I used:

PushNotificationIOS.requestPermissions();
PushNotificationIOS.addEventListener('register', function(devicetoken){
  console.log('register', devicetoken);
  alert('register: ' + devicetoken);
});

PushNotificationIOS.addEventListener('error', function(error){
  var errorKey = Object.keys(error)[0]; 
  var errorValue = error[errorKey];
  console.log('error', errorKey, errorValue);
  alert('error: ' + errorValue);
});

The error event is working but it can be cleaned up. I personally prefer the parameters to be: key, value instead of error but I would like to get thoughts from other people on this. Maybe it is a smart thing to start a separate issue/pull request for this after this one is solved and merged.

I hope this helps. Would love the hear if this is working for you or what isn't.

dorthwein commented 9 years ago

@DannyvanderJagt awesome thanks - with this do you still need to do the above objective-c changes? Testing it today.

DannyvanderJagt commented 9 years ago

@dorthwein If you use my fork then you don't need the above changes. Everything should be ready to go (only the SampleApp for now, because of the appDelegate changes). Let me know if it works for you.

DannyvanderJagt commented 9 years ago

Does anyone know which AppDelegate files I should change to get a pull request accepted? I can't find any AppDelegate that is not part of an example. Thanks.

dorthwein commented 9 years ago

@DannyvanderJagt your solution/branches worked great once I got my NPM stuff in order.

Thanks again!

dorthwein commented 9 years ago

@DannyvanderJagt I looked around a bit for where to contribute to AppDelegate - No luck but I feel like it would be in the react-native-cli code since its the CLI Init that creates that initial file.

Any who here is the NPM link - I suggest reaching out to one of the contributors listed on there.

https://www.npmjs.com/package/react-native-cli

dorthwein commented 9 years ago

I wanted to add another tidbit thats really important. Basically you need to add the corresponding bridge to AppDelegate.m for each JS call you want to use. So if you want to be able to receive notifications you also have to add

- (void)application:(UIApplication *)application didReceiveRemoteNotification:(NSDictionary *)notification
{
  [RCTPushNotificationManager application:application didReceiveRemoteNotification:notification];
}
DannyvanderJagt commented 9 years ago

Thank you, I will add this to my fork. I am planning to take a good look at this issue and create a pull request tomorrow.

DannyvanderJagt commented 8 years ago

I have updated my fork. All the changes are implemented now and everything is working! There is one thing left to do. When some uses my fork without the RCTPushNotification library, a few errors occur because not everything can be found.

I think that there are two solutions for this:

I would prefer the second solution, but I don't know if it is possible. I have looked around and tried some stuff but I can't get this to work.

I have tried to use this but this doens't work for me:

#if __has_include("RCTPushNotificationManager.h")
  // Pushnotification code.
#endif

Does anyone know a solution for this?

jakeyr commented 8 years ago

@DannyvanderJagt thanks for these fixes... are you planning on opening a PR and getting this merged in? Would love to see these fixes in a release.

DannyvanderJagt commented 8 years ago

@jakeyr Thanks and yes I am, I will create a PR this afternoon tomorrow.

Yesterday I tried to find a fix for the last issue (when someone uses the code without the RCTPushNotification library) but I couldn't find a good solution. Someone told me that it is actually a bad practice. Maybe that someone from Facebook can help us figure out what to do with this last issue.

DannyvanderJagt commented 8 years ago

I worked on a pull request today and I looked around for a way to check for the RCTPushNotification and I did found a working solution. I will do some more testing and then I will update my fork and create the Pull Request.

jakeyr commented 8 years ago

Awesome! Thanks Danny. Your fork works great for me.

On Mon, Jul 13, 2015 at 9:44 AM, Danny van der Jagt notifications@github.com wrote:

I worked on a pull request today and I looked around for a way to check for the RCTPushNotification and I did found a working solution. I will do some more testing and then I will update my fork and create the Pull Request.

Reply to this email directly or view it on GitHub: https://github.com/facebook/react-native/issues/1613#issuecomment-120989918

DannyvanderJagt commented 8 years ago

As soon as https://github.com/facebook/react-native/pull/2332 is merged. This issue can be closed.

DannyvanderJagt commented 8 years ago

This issue can be closed. This is fixed in #2332.

vpezeshkian commented 8 years ago

It's been a while I have similar issue but after reading many different solutions I finally decided to ask here, because I wasn't able to fix my issue.

I'm using iOS9.1 and react-native 0.13.0-rc. and no matter what I do didRegisterForRemoteNotificationsWithDeviceToken is never called. Regardless of what library i'm calling under this function ([RCTPushNotificationManager] or [PFInstallation]) it is not called.

I even added that method in AppDelegate.h as suggested by DannyvanderJagt, but no luck.

What am I missing? any help would be appreciated.

marcshilling commented 8 years ago

Is there anyway we can make the 'register' event fire on the simulator, even if it didn't actually register?

I need my app to know whether the user selected Yes or No on the default iOS alert dialog. Is there a better way I should be accomplishing it? I wish it worked on the simulator strictly for testing purposes.

DannyvanderJagt commented 8 years ago

@vpezeshkian That's weird, it is supposed to be working right out of the box now. However there are more people who have difficulties implementing push notifications. I have written a step-by-step tutorial on how to get it working. It helped dozen of people, maybe it helps you too. Let me know if the tutorial didn't work for you and I will help you to get it working.

@marcshilling I get your point. I am not sure if that is something we should implement in React Native because it would basically be a hack around the IOS simulator and IOS itself. But we can always take a look of course!

Maybe it is an option to call for example the register callback in JS separately to simulate a registration.

var onRegister = function(token){
 console.log(‘You are registered and the device token is: ‘,token)
};

// For an actual device.
PushNotificationIOS.addEventListener(‘register’, onRegister);

// For the simulator.
onRegister('copy a device token or create a custom token');
marcshilling commented 8 years ago

@DannyvanderJagt not a big deal...I'll just use react-native-device and have it assume the user pressed "Yes" if on simulator. Definitely need the error event though...hoping they merge your PR soon. Am I correct to assume the error event fires if the user selects "No" in the request pop-up?

DannyvanderJagt commented 8 years ago

@marcshilling Thanks for the tip about react-native-device. I will update and rebase the PR this weekend. I hope they merge it soon but the React Native team is very busy at the moment.

The error event is a bridge for didFailToRegisterForRemoteNotificationsWithError in IOS. The error event will only fire when for example: you are running the app on a simulator or when the certificates aren't configured the right way. But it will not fire when the user selects "No" in the request pop-up.

I did some digging but as far as I could find IOS doesn't have a callback for the select 'No' issue. You can however check the push notifications permission in React-Native but using: checkPermission

tachim commented 8 years ago

@DannyvanderJagt nice tutorial! But "RCTSharedApplication" isn't defined in my checkout of react native 0.11.0 so the bug fix doesn't work.

tachim commented 8 years ago

OK I think the right fix is to replace it with UIApplication *app = [UIApplication sharedApplication];.

DannyvanderJagt commented 8 years ago

@tachim Thank you! And also a thank you for the heads up. I looked into it and saw that In React-Native v0.12 and lower [UIApplication sharedApplication] is indeed replaced with RCTSharedApplication();. I have updated the article with you fix!

codecvlt commented 8 years ago

Is this fixed in RN 0.13.2? Because I am still not getting the "register" or "notification" events to fire.

DannyvanderJagt commented 8 years ago

@karlgodard This is fixed in v0.13 and higher. I have written a step by step tutorial on How to use push notifications in React-Native (IOS). Maybe it can with your problem.

If the tutorial didn't help to fix your problem, please let me know and I will help you. :)

codecvlt commented 8 years ago

Thanks @DannyvanderJagt

Finally got it working. I recently switched from a 3rd party solution react-native-remote-push back to the PushNotificationIOS, since the issue was said to be fixed. Had a couple wires crossed in that process, which I've fixed, so it's all good now.

Thanks for the quick response!

niftylettuce commented 8 years ago

Still not working.

niftylettuce commented 8 years ago

screen shot 2015-11-07 at 8 17 30 pm

niftylettuce commented 8 years ago

Okay, fixed it. you have to Open the RCTPushNotification project in XCode, then CMD+B to rebuild. Before rebuilding, make sure you have this folder linked in the Search headers path and also added to linked libraries.

alvinwoon commented 8 years ago

@DannyvanderJagt I followed your guide, which is very well written btw, but the register event still didn't get fired. The cert works, library linked just fine, app compiled just fine, using RN 14.2, ios 9.1 and latest xcode.

Still trying to figure out how to get device token via the register event.

alvinwoon commented 8 years ago

Finally got it working. For those who still couldn't get the register event working:

1.) Clean up your provisioning profiles. Even better generate new profiles with the new cert and apn and delete the rest from your device and xcode. 2.) Clean project and run again.

http://stackoverflow.com/a/8036052/4563055

DannyvanderJagt commented 8 years ago

@alvinwoon That's a great solution, thank you. I did always change the name of my app to test the notification alert. I'm sorry for my somewhat late response, glad you figured it out. I will add this to the article if you that's oke with you.

alvinwoon commented 8 years ago

@DannyvanderJagt for sure.

skevy commented 8 years ago

We use push notifications extensively in our RN apps, and based on the comments here, it seems like it's working correctly from a React Native perspective.

Closing this issue for now, but please post on stack overflow if you all have any questions!

vmakhaev commented 8 years ago

Push notifications on IOS does not work in Simulator. To see an error, you need to register this delegate