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

Add IOSMessaging module for APNS specific methods #1626

Merged
merged 14 commits into from
Oct 25, 2018
Merged

Add IOSMessaging module for APNS specific methods #1626

merged 14 commits into from
Oct 25, 2018

Conversation

timwangdev
Copy link
Contributor

@timwangdev timwangdev commented Oct 23, 2018

Summary

Fixes #1511

The user should have the freedom when to call the registerForRemoteNotifications from application instance as described in firebase doc https://firebase.google.com/docs/cloud-messaging/ios/client#register_for_remote_notifications

And expose APNS token is very handy for debugging device token sync issue in iOS. This is not a standard firebase method.

Checklist

Test Plan

N/A

Release Plan

[IOS] [FEATURE] [MESSAGING] - Add IOSMessaging module for APNS specific methods.


Think react-native-firebase is great? Please consider supporting the project with any of the below:

@timwangdev
Copy link
Contributor Author

I will add the doc if this passed the initial review.

@codecov
Copy link

codecov bot commented Oct 23, 2018

Codecov Report

Merging #1626 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1626   +/-   ##
=======================================
  Coverage   70.99%   70.99%           
=======================================
  Files          71       71           
  Lines        1834     1834           
=======================================
  Hits         1302     1302           
  Misses        532      532

@Salakar Salakar self-assigned this Oct 23, 2018
@Salakar Salakar self-requested a review October 23, 2018 08:57
@Salakar Salakar added the plugin: messaging FCM only - ( messaging() ) - do not use for Notifications label Oct 23, 2018
@Salakar Salakar added this to the v6.0.0 milestone Oct 23, 2018
@timwangdev timwangdev changed the title Fix #1511: Add registerForRemoteNotifications method Fix #1511 Add registerForRemoteNotifications method Oct 23, 2018
@@ -64,6 +64,11 @@ public void requestPermission(Promise promise) {
promise.resolve(null);
}

@ReactMethod
public void registerForRemoteNotifications(Promise promise) {
Copy link
Member

@Salakar Salakar Oct 23, 2018

Choose a reason for hiding this comment

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

We can remove this and add an if (isAndroid) return; line in the JS method, native code changes not required then 👍

Copy link
Member

@Salakar Salakar left a comment

Choose a reason for hiding this comment

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

Thanks for sending this over, just a couple of small things on the review, thanks

@@ -137,6 +137,10 @@ export default class Messaging extends ModuleBase {
return getNativeModule(this).hasPermission();
}

registerForRemoteNotifications(): Promise<void> {
Copy link
Member

Choose a reason for hiding this comment

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

Could we namespace this under ios? It's good to keep platform-specific methods outside of the main cross-platform methods.

e.g. this becomes firebase.messaging().ios.registerForRemoteNotifications()

@Salakar Salakar modified the milestones: v6.0.0, v5.1.0 Oct 23, 2018
@timwangdev timwangdev changed the title Fix #1511 Add registerForRemoteNotifications method Add registerForRemoteNotifications method Oct 23, 2018
@timwangdev timwangdev changed the title Add registerForRemoteNotifications method Add IOSMessaging module for APNS specific methods Oct 24, 2018
@timwangdev
Copy link
Contributor Author

@Salakar I just merged #1625 to this branch since the function is similar and shared the same structure.

@Salakar
Copy link
Member

Salakar commented Oct 24, 2018

@timwangdev this is great, thanks for updating. Would you be able to send a docs PR over also (on master branch) please;

You can add a properties and iOS section on here: https://github.com/invertase/react-native-firebase-docs/blob/master/docs/messaging/reference/Messaging.md

You can add a new IOSMessaging.md file here: https://github.com/invertase/react-native-firebase-docs/tree/master/docs/messaging/reference

and then add it to the menu here after this line: https://github.com/invertase/react-native-firebase-docs/blob/master/docs/_sidebar.yaml#L203

You have collab rights on the docs repo so you can create a branch and do it on the online GH editor if you wish, see https://github.com/invertase/react-native-firebase/blob/master/CONTRIBUTING.md#our-expectations-on-you-as-a-contributor

You can change swap out the version in the URL for the docs site to your branch name to preview, e.g. https://rnfirebase.io/docs/YOUR_BRANCH_NAME_HERE/notifications/reference/IOSNotification#badge

@timwangdev
Copy link
Contributor Author

Added doc at invertase/react-native-firebase-docs#137

@Salakar
Copy link
Member

Salakar commented Oct 25, 2018

@timwangdev thanks for sorting the docs, and this PR, nice 👍 will merge now 🎉

@Salakar Salakar merged commit 6858137 into invertase:master Oct 25, 2018
@timwangdev timwangdev deleted the fix-register-apns branch October 25, 2018 05:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: ios plugin: messaging FCM only - ( messaging() ) - do not use for Notifications
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Registering for notifications is coupled with the modal requestPermission
2 participants