Pushnotifications events and fix for AppDelegate

[DannyvanderJagt]

There are a few problems with the push notifications in React-Native, which I discovered when I wanted to use push notifications for a project. I did some digging and I found out that there were some issues (#1304, #1019) and that some people created a workaround. This PR should fix those issues including the issue (#1613) where some good folks and I disused this.

Issue 1:

• Push notifications are working in React-Native but not out of the box. There are some things that people should add to their xcode project but there is nothing documented, besides adding the library.

At this point people should:
1) add RCTPushNotification to their project.
2) add this header to their Header Search Paths: $(SRCROOT)/node_modules/react-native/Libraries/**
3) add this code to their AppDelegate.m:

#import "RCTPushNotificationManager.h"

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

- (void)application:(UIApplication *)application didReceiveRemoteNotification:(NSDictionary *)notification
{
    [RCTPushNotificationManager application:application didReceiveRemoteNotification:notification];
}

Solution
I couldn't find any documentation about this issue so this should be added to this page.
But I don't like it that people have to add a bunch of Objective-c code (Objective-c developers aren't the target audience here I think), so I did my best to create a good solution for this. (See 4c0ba9e)

I have added the code (from above) to AppDelegate.m and created a check for RCTPushNotification. With this check the code will only run when RCTPushNotification is available/added and projects without RCTPushNotification won't notice any changes. The only thing people have to do is to add the library and start using PushNotificationIOS in JS.

Issue 2:
Update: This is now a seperate PR: #2336

Push notifications doesn't have error handling and therefore all the errors that occur are silenced. Xcode doesn't show any errors and there is no error event in JS. So when people use it and they didn't do everything right the first time they are left in the dark.

Solution:
I have created an error event for javascript. This event will be fired when an error occurs in IOS and it will give you the error key and the error message straight from IOS. In IOS the error is an NSError which means that javascript gets it this way: {key: message}. I did separate the key and the message so that you can use the error event this way:

PushNotificationIOS.addEventListener('error', function(key, message){
  console.log('error',  key,  message);
  alert('error: ' + message);
});

How do you guys think about this? I would love the get your opinions/code reviews on this and to get this working in React-Native!

[facebook-github-bot]

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

[facebook-github-bot]

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

[sahrens]

Agreed we can make this much easier, but I'm not sure this is the right approach...would it be possible to use a category instead so we don't have to add anything to AppDelegate.m? Passing to @tadeuzagallo

[tadeuzagallo]

It used to be documented on the sources, but apparently it's gone...

@sahrens a category wouldn't work because the class name may change. I think I'm okay with this, since it's only on the SampleApp, I think it's harmless, but /cc @nicklockwood to see what his thoughts on it.

[brentvatne]

This tripped us up as well, I think the main issue right now is just this one:

2. add this header to their Header Search Paths: $(SRCROOT)/node_modules/react-native/Libraries/**

We could either add react-native/Libraries to the search paths on the SampleApp OR change the search path on the sample app from react-native/Libraries to react-native/React

cc @mattmo

[DannyvanderJagt]

Good to hear some thoughts on this.

The solution from @brentvatne makes it less tricky to get up and running and with some proper documentation then it shouldn't be a problem. However I do think that preventing the user from adding code would be the best solution for the user/developer but only if it can be done in a good way. What are your thoughts on this?

The possible solution in this PR is based on the issues I mentioned, the conversation in #1019 and the comment from @nicklockwood #1019 (comment)

With a bit of runtime logic we could just detect if the RCTPushNotificationManager exists, and include the necessary code to call it in the AppDelegate of the standard project template. Maybe that's the best option.

I think that the code changes from the PR are what he meant, but I'm not entirely sure. I would like to hear his thoughts on this.

@sahrens We could extend AppDelegate. This way we don't have to add anything to AppDelegate.m.
Actually there is a PR which does that (#1019) but it can be cleaned up a little. I have tested it and it did work for me however in my version I moved the two extra files to RCTPushNotification and in some cases it will call the didFailToRegisterForRemoteNotificationsWithError twice. But I think that it can be fixed.

Whatever the solution will be I do think that it should not only be applied to the SampleApp but also to the standaard project template, as nick said, because most people will use this to start their projects.

[DannyvanderJagt]

Any updates or thoughts on this?

[bogdantmm92]

+1

[jaygarcia]

Question: does this have anything to do with JSContext not executing while the phone is locked & screen is off? I ask because I've seen a relationship discussed between the two problems.

[DannyvanderJagt]

@jaygarcia Not that I know off. The didRegisterForRemoteNotificationsWithDeviceToken doesn't get fired in Objective-c (without it being in the AppDelegate file) so there is no JS involved.

@brentvatne @tadeuzagallo @sahrens At this point I think is smart to separate this PR to keep things moving.

I would like to separate this PR into:

• a PR with a fix for IOS 7 & 8 (see RCTPushNotificationManager.m) line: 159
• a PR which adds an error event to PushNotificationIOS

And clean this one up and leave this one open for discussing about the Appdelegate fix/documentation fix. Or is there a way I can help you guys with the documentation for this?

Please let me know what you think or what I can do.

[jaygarcia]

Thanks @DannyvanderJagt, for the clarification :). I thought these might be separate issues, but wanted to be sure.

[eyaleizenberg]

@DannyvanderJagt Any updates on this? Would love to see this merged. Thanks!

[ghost]

👍 changing RCTPushNotificationManager.m 159 works for me! Thanks @DannyvanderJagt Would love to see this merged too

[DannyvanderJagt]

@eyaleizenberg There is a conversation about the changes that I made in Appdelete, but there is no clear answer from the collaborators. So I will split this PR into:

• a PR with a fix for IOS 7 & 8 (see RCTPushNotificationManager.m) line: 159
• a PR which adds an error event to PushNotificationIOS

And I will leave this PR open and clean it up.
I am a little bit busy at the moment with work and family, so it can take a few days.

@potsypantsy Thank you for confirming!

[ghost]

@DannyvanderJagt if you don't mind I created a PR with just the changes in RCTPushNotificationManager.m

[DannyvanderJagt]

@potsypantsy No problem! Thank you for helping :)

[eyaleizenberg]

@DannyvanderJagt Were you able to:

1. Get the device token when requesting permissions?
   I added the following code to my index.ios.js file:

PushNotificationIOS.addEventListener('register', (value) => {
   console.log(value)
});
PushNotificationIOS.requestPermissions();

And indeed I get the prompt that the app is requesting permission but the eventListener never triggers and I don't know the device's token. If I run:

PushNotificationIOS.checkPermissions((permissions) => {
   console.log(permissions);
});

I can see that I do have the permission, but again, I don't know the device's token.

1. Revoke the permissions so you could ask for them again? I tried running PushNotificationIOS.abandonPermissions() but nothing happens. If I checkPermissions aferwards, I still have all the permissions. This sucks because I can't test the register listener several times.

Thanks.

[eyaleizenberg]

@DannyvanderJagt By the way, when using your code, the following warning appears in xcode.

[DannyvanderJagt]

@eyaleizenberg In my fork I was able to get the device token using the register event, but maybe something changed in the master. The warning is a type related waring, nothing serious for now.
I will take a look at it and try to fix it. I will post a link to my new branch when I'm done!

I didn't know about the problems with abandonPermissions and I saw that you have created a separate issue for that. I will take a look at it, maybe I can help you.

Thank you for pointing this out!

[DannyvanderJagt]

I updated the code for this pull request but I accidentally closed the PR by removing the last few commits in order to rebase. Is there a way I can reopen it. I have already push new code to this branch.

@eyaleizenberg The register event is working here. But in order to make the register event fire there are a few things that has to be configured in the right way:

• All your certificates must be available and configured in the right way.
• The user must have given permission. In the settings app the "Allow Notifications" should be turned on.
• The changes from this PR should be made. [PushNotificationIOS] fixes requestPermissions base on @DannyvanderJagt… #2302

All those things have to be done the right way otherwise the register event won't fire.
I you want you can use the branch I used with the changes from PR 2302 - See my branch

I found it very difficult to get the certificates right without seeing any error messages from react-native or IOS. I will create a PR with an error event for the push notifications module in React native today. The error event will help you to get those things right.

For the record: The question about abandonPermissions is answered here: #2280

[cyprusglobe]

@DannyvanderJagt I'm wondering if you could write a tutorial on how to get push notifications working step by step, i think this would clear alot of confusion.

[DannyvanderJagt]

@cyprusglobe That would be a great idea! Any suggestions on where I should post the tutorial?

[cyprusglobe]

maybe just a blog post or a https://medium.com/ post or a video tutorial.

[DannyvanderJagt]

I have written a tutorial about How to use Push Notifications in React-Native. It's a step by step tutorial and everything from adding the library to getting the right certificates is explained. I hope it helps to clear the confusion about push notifications in React-Native. Let me know if I missed something.

I have also mentioned these two Pull Requests (#2332, #2336) in the tutorial. I think that these PRs are important to get push notifications stable and fully working in React-Native.

@sahrens @tadeuzagallo @brentvatne @nicklockwood Can you help to get those PRs (#2332, #2336) merged? Pull notifications aren't working right now in IOS 8 and higher and the error event would be a big improved for the people how want to use/are using push notifications in React-Native.

[cyprusglobe]

Thanks @DannyvanderJagt

[yogurt-island-331]

@DannyvanderJagt amazing blog post! Thank you so much! Just curious, was there a reason that the following code was not committed into the latest release?

if ([[UIApplication sharedApplication] respondsToSelector:@selector(registerUserNotificationSettings:)]) {
    // IOS 8 and later.
    id notificationSettings = [UIUserNotificationSettings settingsForTypes:types categories:nil];
    [[UIApplication sharedApplication] registerUserNotificationSettings:notificationSettings];
    [[UIApplication sharedApplication] registerForRemoteNotifications];
  }else{
    [[UIApplication sharedApplication] registerForRemoteNotificationTypes:types];
  }

[DannyvanderJagt]

@kevinzzz007 Thank you!

That piece of code is part of this PR #2332 which is merged a few weeks ago. The fix is implemented in all the versions from now on (v0.13 and higher). If you use v0.13 and higher you should be fine, otherwise you still have to implement the bug fix yourself.

Thank you for the information. I have updated the article!

[yogurt-island-331]

Ahh I see! Thanks for letting me know and the the bug fix, extremely
helpful! I am amazed by the React Native community everyday

On Monday, October 19, 2015, Danny van der Jagt notifications@github.com
wrote:

@kevinzzz007 https://github.com/kevinzzz007 Thank you!

That piece of code is part of this PR #2332
#2332 which is merged a
few weeks ago. The fix is implemented
https://github.com/facebook/react-native/blob/0.13-stable/Libraries/PushNotificationIOS/RCTPushNotificationManager.m#L159-L167
in all the versions from now on (v0.13 and higher). If you use v0.13 and
higher you should be fine, otherwise you still have to implement the bug
fix yourself.

Thank you for the information. I have updated the article!

—
Reply to this email directly or view it on GitHub
#1979 (comment)
.

Kevin Zhao | LinkedIn http://www.linkedin.com/in/kevinzhaouva | Personal
Blog https://medium.com/@kevinzhao | Github
https://github.com/kevinzzz007
(434) 282-5369
Computer Science and Information Technology | University of Virginia 2016
Alpha Kappa Psi Professional Business Fraternity
http://www.akpsiatuva.org/