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

[PushNotifications] Add an event for remote notification registration, and improve permissions request #1304

Closed
wants to merge 2 commits into from

Conversation

andrewimm
Copy link
Contributor

In order to add Push support to the Parse JS SDK in React Native, we need a way to receive the APNS device token from the JS context. This adds another event to PushNotificationIOS, so that code can respond to a successful registration.

Additionally, I've updated the requestPermissions call to accept an optional map of parameters. This way, developers can request a subset of user notification types.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 15, 2015
@ericvicenti
Copy link
Contributor

@facebook-github-bot import

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to https://our.intern.facebook.com/intern/opensource/github/pull_request/1067134919980570/int_phab to review.

@andrewimm
Copy link
Contributor Author

Squashed and rebased

@ericvicenti
Copy link
Contributor

Just tried to run this on the iPhone 6 simulator and it couldn't build. Looks like your enum is not compatible with iOS8

@lazywei
Copy link

lazywei commented May 20, 2015

I think if this is merged, then #1176 is closable. I'm looking forward to this being merged 👍
Thanks.

@@ -52,6 +57,21 @@ + (void)application:(UIApplication *)application didRegisterUserNotificationSett
}
}

+ (void)application:(UIApplication *)application didRegisterForRemoteNotificationsWithDeviceToken:(NSData *)deviceToken
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I trying to use this fork, this line seems to be never called. Did I miss anything?

@lazywei
Copy link

lazywei commented May 20, 2015

I'm not sure if I miss anything, but it seems to me the didRegisterForRemoteNotificationsWithDeviceToken is never called.
For anyone is interested in this, my workaround is adding these lines into your iOS/AppDelegate.m, just below the didFinishLaunchingWithOptions:

- (void)application:(UIApplication *)application didRegisterForRemoteNotificationsWithDeviceToken:(NSData *)deviceToken
{
  NSMutableString *hexString = [NSMutableString string];
  const unsigned char *bytes = [deviceToken bytes];

  for (int i = 0; i < [deviceToken length]; i++) {
    [hexString appendFormat:@"%02x", bytes[i]];
  }
  NSDictionary *userInfo = @{
    @"deviceToken" : [hexString copy]
  };
  [[NSNotificationCenter defaultCenter] postNotificationName:@"RemoteNotificationsRegistered"
                                                      object:self
                                                    userInfo:userInfo];
}

@lazywei
Copy link

lazywei commented May 22, 2015

ping @ericvicenti , how is it going?
actually, I can build this on iPhone5 / simulator (both are iOS8)

@andrewimm
Copy link
Contributor Author

@ericvicenti I'm assuming your comment was a typo, since I was having no trouble running on iOS 8 in the simulator or on device, but there were linker & enum errors with iOS 7.1. I made some adjustments to get the linker to cooperate again, things are working now on 7 and 8.

@lazywei you're right, you need to call the public didRegisterForRemoteNotificationsWithDeviceToken from the method of the same name in your AppDelegate.m. This is by design -- there is no way to have a linked library automatically provide this method for your AppDelegate without overriding AppDelegate itself. My personal recommendation is to add the following definition to AppDelegate.m:

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

You'll need to do the same thing if you want to respond to didReceiveRemoteNotification

@lazywei
Copy link

lazywei commented May 23, 2015

@andrewimm Great, thanks! I think this should be remarked on the doc, right? By the way, when doing this (I mean, calling RCTPushNotificationManager), do I need to include any header file in my AppDelegate.m?

@d-vine
Copy link

d-vine commented May 31, 2015

@lazywei Being a total Objectiv-C n00b I struggled with the headers. To get it to work one needs to
#import "RCTPushNotificationManager.h"
in AppDelegate.m. You also need to make sure, that
$(SRCROOT)/node_modules/react-native/Libraries (recursive)
is part of your projects header search path.

@brentvatne brentvatne changed the title Add an event for remote notification registration, and improve permissions request [PushNotifications] Add an event for remote notification registration, and improve permissions request Jun 1, 2015
@lazywei
Copy link

lazywei commented Jun 3, 2015

ping @ericvicenti how is this going?
i notice there is another similar PR #1019
should we merge these two issues?
I think push notification's device token is one of the core functionality, and thus it would be great if we can merge this kind of PR earlier. Otherwise, there will be a lot of duplicated works showing on the GitHub issues list.

@ericvicenti
Copy link
Contributor

@lazywei @andrewimm

Sorry for all the delay on this! I'm not very familiar with iOS notifs and I had to fix an issue with iOS7/8 notif registration types.

Good news is it has landed internally and will be synced to GH tomorrow morning! Thanks for all your patience on this

@d-vine
Copy link

d-vine commented Jun 4, 2015

@ericvicenti
Great news. Thanks!

@niftylettuce
Copy link
Contributor

Can we please update the docs at https://facebook.github.io/react-native/docs/pushnotificationios.html to show users how to get a token that's already been registered?

@niftylettuce
Copy link
Contributor

Figured it out - we should add a line:

Every time the app is re-opened, the register event gets triggered if push notifications are enabled.

@kevinaltschuler
Copy link

I'm on 0.14.0-rc and the event doesnt seem to ever get triggered

in my index.ios.js

  componentWillMount() {
    PushNotificationIOS.addEventListener('register', this._onNotificationReg);
  },
  _onNotificationReg(data) {
    console.log(data);
  },

@d-vine
Copy link

d-vine commented Nov 15, 2015

Hi @Kevtastic. Did you add the listener functions to your AppDelegate.m?

@kevinaltschuler
Copy link

i did yeah, my appdelegate looks like this right now

/**
 * Copyright (c) 2015-present, Facebook, Inc.
 * All rights reserved.
 *
 * This source code is licensed under the BSD-style license found in the
 * LICENSE file in the root directory of this source tree. An additional grant
 * of patent rights can be found in the PATENTS file in the same directory.
 */

#import "AppDelegate.h"
#import "RCTPushNotificationManager.h"
#import "RCTRootView.h"
#import "RCTLinkingManager.h"

@implementation AppDelegate

- (BOOL)application:(UIApplication *)application openURL:(NSURL *)url sourceApplication: (NSString *)sourceApplication annotation: (id)annotation {
    return [RCTLinkingManager application:application openURL:url sourceApplication:sourceApplication annotation:annotation];
  }

// push notification functions

// Required for the register event.
- (void)application:(UIApplication *)application didRegisterForRemoteNotificationsWithDeviceToken:(NSData *)deviceToken
{
  [RCTPushNotificationManager application:application didRegisterForRemoteNotificationsWithDeviceToken:deviceToken];
}
// Required for the notification event.
- (void)application:(UIApplication *)application didReceiveRemoteNotification:(NSDictionary *)notification
{
  [RCTPushNotificationManager application:application didReceiveRemoteNotification:notification];
}

- (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(NSDictionary *)launchOptions
{
  NSURL *jsCodeLocation;

then my js bundling stuff

@d-vine
Copy link

d-vine commented Nov 15, 2015

I think you also need to call PushNotificationIOS.requestPermissions() at least once.
I do it every app startup, because if you already have the permissions, nothing happens.

@kevinaltschuler
Copy link

I'm calling requestPermissions inside of render right now, though I've tried to do it in componentWillMount and in componentDidMount.

I've tried listening for the event before and after I request permissions but in both cases im not getting anything

@ivanbrens
Copy link

+1 same issue as @Kevtastic

@kevinaltschuler
Copy link

@ivanbrens
my issue was that I was trying to test on the simulator with development certificates.

As soon as I switched over to production certs and testflight it started working.

I'm convinced development certificates have never worked for anyone ever.

@andrewimm
Copy link
Contributor Author

@Kevtastic development certs definitely worked when this code landed -- otherwise, I wouldn't have been able to test it. However, Apple has never supported notification registration in a Simulator, which is probably why things never worked until you moved to a physical device. It's unfortunate, but part of the iOS development experience. Thankfully, it's much easier to deploy to a physical device these days, now that app deploys can be tied to your iCloud account.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants