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

APNS & Topics #106

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

cbaggers
Copy link
Collaborator

This PR builds on #82 with the principle commit there fixed up according to the feedback from @kusma on the previous PR.

The following 2 commits add support for subscribing to topics and using APNS from Firebase.

The topic support is in this PR as I found it to be pretty much a prerequisite for handling APNS well due to the caveat noted next.

The caveat is that apns does not handle pure 'data messages' from firebase as one would hope. Therefore you should send data messages to android (as the Fuse integration requires it there) and notification messages to iOS. This was you get the same behavior on both platforms. To target the platforms separately I found it helpful to use the topics feature.

I also fixed up a few things I mentioned as being potential issues on the previous PR.

devadiab and others added 3 commits September 29, 2018 21:33
On important caveat is that apns does not handle pure 'data messages'
from firebase as one would hope. It is advisable to use seperate
topics for iOS and android and send data messages to android and
notification messages to iOS. This was you get the same behaviour on
both platforms.

- We remove the static constructors from Common.uno as these wont be
  fired until the class is touched and that could be very late in
  startup (we need to be doing setup during iOS start)
- We add a static constructor to JS.uno as the class is touched during
  startup and we use this to call the Init method in Common.uno
- We disable firebase's method swizzling so we can handle it ourselves
- FireNotificationCallbacks now implements
  UNUserNotificationCenterDelegate so we can set up the system
  correctly for iOS 10 and higher
- RegisterForPushNotifications now registers for notifications in the
  way expected by firebase's documentation
kusma
kusma previously requested changes Sep 30, 2018
Copy link
Contributor

@kusma kusma left a comment

Choose a reason for hiding this comment

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

The first commit here seems to still have the problem that a bunch of code is duplicated without any reasonable explanation that I can see.

@cbaggers
Copy link
Collaborator Author

The first was meant to be android only. Due to Firebase notifications being pretty much useless on android as they cant be delivered when the app is closed. Its designed to be used in conjunction with Fuse.APNS for those who want to handle both separately.

The second has implementations for Android and iOS and is intended to be used with Firebase's push via APNS feature (or whatever the official name is, I can look it up if needed).

The naming is crappy and I happy for that to change. I'd quite like to avoid the duplication too, perhaps detect when Fuse.APNS is in the build and not include the iOS function. However Fuse.Firebase and Fuse.APNS cant be used together if both try hooking into the same AppDelegate callbacks on the same build.

@kusma kusma dismissed their stale review September 30, 2018 22:01

If baggers is happy with this, then I won't object

@CapsAdmin
Copy link

On iOS I believe there's an issue with RegisterForPushNotifications being called before Fire.Core.Init.

Running our project with -FIRDebugEnabled it complains about not being configured. Calling Init at the start of RegisterForPushNotifications fixes this issue for me and I'm able to receive notifications.

I was also confused by OnRegistrationSucceedediOS which doesn't seem to be doing anything. Maybe the code should be removed?

I'll test this some more as I need push notifications for android and ios both inside and outside the app.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants