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] Register for Remote Notifications and Extend AppDelegate #1019

Closed
wants to merge 2 commits into from
Closed

[PushNotifications] Register for Remote Notifications and Extend AppDelegate #1019

wants to merge 2 commits into from

Conversation

lazaronixon
Copy link

Added optional callback to requestPermissions Method.
If registration is successful, APNs returns a device token.
If there is a problem in obtaining the token, a error message is returned.

    PushNotificationIOS.requestPermissions(function(error, token) {
      if (error) {
        console.log(error);
      } else {
        console.log(token);
      }
    });

Extend AppDelegate so you don’t need to call methods on AppDelegate.h

Added optional callback parameter to requestPermissions Method and
Extend AppDelegate so you don’t need to call methods on AppDelegate.h
@facebook-github-bot
Copy link
Contributor

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

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

@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 Apr 26, 2015
@brentvatne
Copy link
Collaborator

@lazaronixon - I definitely like the improvement in the API here, are there any drawbacks to this approach? ObjC isn't my strongest language - is this automatically imported into AppDelegate in every app or will it be opt-in via manual import of the +notification category header?

cc @nicklockwood @tadeuzagallo

@whatupdave
Copy link

BTW I'm using this fork in my app, works great.

@tadeuzagallo
Copy link
Contributor

This is pretty fragile... What if the person renames the file? or the folder? or move the library? This would also break any example app...

When I wrote it I tried swizzling as well, but it doesn't work because we don't know the AppDelegate's class at load time, we thought about some other options, but none of them seemed to be good.

Something that I'm not sure (cc @nicklockwood in case you don't know as well) is whether didRegisterForRemoteNotificationsWithDeviceToken: gets called every time you call registerForRemoteNotifications, and if it behaves in the exact same way in iOS7 & 8. In case it does, you'll still have to cleanup the callbacks on the JavaScript side.

@lazaronixon
Copy link
Author

You're right @tadeuzagallo it is a price to pay, but too high to me, a solution i found was #if __has_include, if file is not found it will work as today and you will need to call methods on the library in ObjC your self. Other solution could be AppDelegate.h come with methods calls to NotificationIOS and it be added by default as other libraries. I feel really bad about need to write ObjC using react-native =/

@nicklockwood
Copy link
Contributor

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.

@lazaronixon
Copy link
Author

If I move AppDelegate+notification to SampleApp Folder and with #if __has_include("RCTPushNotificationManager.h") implement call methods to RCTPushNotificationManager? I would add NotificationIOS to SampleAPP as other libraries too.

@brentvatne brentvatne changed the title Register for Remote Notifications and Extend AppDelegate [PushNotifications] Register for Remote Notifications and Extend AppDelegate Jun 1, 2015
@ghost
Copy link

ghost commented Aug 4, 2015

Thank you for reporting this issue and appreciate your patience. We've notified the core team for an update on this issue. We're looking for a response within the next 30 days or the issue may be closed.

@tadeuzagallo
Copy link
Contributor

@nicklockwood should we add this code to the SampleApp's AppDelegate, or should I close it for now?

@DannyvanderJagt
Copy link
Contributor

@tadeuzagallo @nicklockwood This PR consists of 3 discussions which are continued here:

Extend the appdelegate
There is another PR about this: #1979
This PR does exactly what nick suggested but there is also some discussing whether is should be added.

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.

Device token
This is already available. When you use the register event the devicetoken will be passed as an argument.

PushNotificationIOS.addEventListener('register', function(token){
   console.log('You are registered and the device token is: ',token)
});

Error handling
At the moment PushNotificationIOS doesn't have any error handling. The way this PR is implementing the error handling is by callback. I personally would prefer an error event instead of a callback.The event is cleaner and in line with the current api.
I have created a PR for this: #2336, it is also discussed here: #1613 and here #1979

I think that this PR can be closed.

@satya164
Copy link
Contributor

@lazaronixon Any updates on this?

@facebook-github-bot
Copy link
Contributor

@lazaronixon updated the pull request.

@satya164
Copy link
Contributor

Closing since no activity on the PR. let's re-open if you wanna work on it again.

@satya164 satya164 closed this Jan 26, 2016
ayushjainrksh pushed a commit to MLH-Fellowship/react-native that referenced this pull request Jul 2, 2020
Update touchablehighlight.md
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