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

[PushNotificationIOS] No register event ever gets triggered #1613

Closed
timminkov opened this issue Jun 13, 2015 · 46 comments
Closed

[PushNotificationIOS] No register event ever gets triggered #1613

timminkov opened this issue Jun 13, 2015 · 46 comments
Labels
Resolution: Locked This issue was locked by the bot.

Comments

@timminkov
Copy link

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
Copy link
Contributor

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 lazaronixon@af08fde 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
Copy link
Author

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

@DannyvanderJagt
Copy link
Contributor

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 #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
Copy link
Author

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

@DannyvanderJagt
Copy link
Contributor

@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
Copy link
Author

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link

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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link

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

@DannyvanderJagt
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link

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

Thanks again!

@dorthwein
Copy link

@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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor

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:

  • Let the user add the code to the AppDelegate file when he/she wants to use push notifications. (This solution is used by phonegap/cordova and a lot of other plugins)
  • Or find a way to check when the RCTPushNotification library is available.

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
Copy link

jakeyr commented Jul 12, 2015

@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
Copy link
Contributor

@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
Copy link
Contributor

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
Copy link

jakeyr commented Jul 13, 2015

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:
#1613 (comment)

@DannyvanderJagt
Copy link
Contributor

As soon as #2332 is merged. This issue can be closed.

udfalkso pushed a commit to purposecampaigns/react-native that referenced this issue Oct 7, 2015
Summary: **Problem**
Using push notifications in IOS 8 will throw this error:
`registerForRemoteNotificationTypes: is not supported in iOS 8.0 and later.`
The problem is that the check is running on compile instead of runtime.

**Solution**
If have changed the compile if statement to a runtime if statement. The fix is tested on: IOS 7.1 and 8.* and everything is working now.
This solution is also discussed in: facebook#1613 and it was part of  facebook#1979. (is being separated to keep things moving)

Please let me know what you think.Closes facebook#2332

Reviewed By: @​svcscm

Differential Revision: D2490987

Pulled By: @ericvicenti
philly-d pushed a commit to philly-d/react-native that referenced this issue Oct 16, 2015
Summary: **Problem**
Using push notifications in IOS 8 will throw this error:
`registerForRemoteNotificationTypes: is not supported in iOS 8.0 and later.`
The problem is that the check is running on compile instead of runtime.

**Solution**
If have changed the compile if statement to a runtime if statement. The fix is tested on: IOS 7.1 and 8.* and everything is working now.
This solution is also discussed in: facebook#1613 and it was part of  facebook#1979. (is being separated to keep things moving)

Please let me know what you think.Closes facebook#2332

Reviewed By: @​svcscm

Differential Revision: D2490987

Pulled By: @ericvicenti
@DannyvanderJagt
Copy link
Contributor

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

@vpezeshkian
Copy link

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
Copy link

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
Copy link
Contributor

@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
Copy link

@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
Copy link
Contributor

@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
Copy link

tachim commented Oct 29, 2015

@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
Copy link

tachim commented Oct 29, 2015

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

@DannyvanderJagt
Copy link
Contributor

@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
Copy link

codecvlt commented Nov 1, 2015

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

@DannyvanderJagt
Copy link
Contributor

@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
Copy link

codecvlt commented Nov 1, 2015

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
Copy link
Contributor

Still not working.

@niftylettuce
Copy link
Contributor

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

@niftylettuce
Copy link
Contributor

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.

MattFoley pushed a commit to skillz/react-native that referenced this issue Nov 9, 2015
Summary: **Problem**
Using push notifications in IOS 8 will throw this error:
`registerForRemoteNotificationTypes: is not supported in iOS 8.0 and later.`
The problem is that the check is running on compile instead of runtime.

**Solution**
If have changed the compile if statement to a runtime if statement. The fix is tested on: IOS 7.1 and 8.* and everything is working now.
This solution is also discussed in: facebook#1613 and it was part of  facebook#1979. (is being separated to keep things moving)

Please let me know what you think.Closes facebook#2332

Reviewed By: @​svcscm

Differential Revision: D2490987

Pulled By: @ericvicenti
@alvinwoon
Copy link

@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
Copy link

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
Copy link
Contributor

@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
Copy link

@DannyvanderJagt for sure.

@skevy
Copy link
Contributor

skevy commented Dec 15, 2015

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!

@skevy skevy closed this as completed Dec 15, 2015
Crash-- pushed a commit to Crash--/react-native that referenced this issue Dec 24, 2015
Summary: **Problem**
Using push notifications in IOS 8 will throw this error:
`registerForRemoteNotificationTypes: is not supported in iOS 8.0 and later.`
The problem is that the check is running on compile instead of runtime.

**Solution**
If have changed the compile if statement to a runtime if statement. The fix is tested on: IOS 7.1 and 8.* and everything is working now.
This solution is also discussed in: facebook#1613 and it was part of  facebook#1979. (is being separated to keep things moving)

Please let me know what you think.Closes facebook#2332

Reviewed By: @​svcscm

Differential Revision: D2490987

Pulled By: @ericvicenti
@vmakhaev
Copy link

vmakhaev commented May 6, 2016

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

@facebook facebook locked as resolved and limited conversation to collaborators Jul 22, 2018
@react-native-bot react-native-bot added the Resolution: Locked This issue was locked by the bot. label Jul 22, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Locked This issue was locked by the bot.
Projects
None yet
Development

No branches or pull requests