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

[Discussion] Where to go with the Parse Push for Android #572

Closed
inlined opened this issue Jan 16, 2017 · 14 comments
Closed

[Discussion] Where to go with the Parse Push for Android #572

inlined opened this issue Jan 16, 2017 · 14 comments

Comments

@inlined
Copy link

inlined commented Jan 16, 2017

Given the coming shutdown, I think it's time to discuss the plans that would replace my old attempt at fixing Push. What should change in the Parse SDK? The new FCM SDKs have all the right bindings so the Parse SDK can be built to work natively with FCM without requiring the compile-time dependency on GMS. It'd be a bit of dead code when users don't use Push and very thin stringly-typed bindings when it is there. The recommendations I'm feel are clear:

  1. Rip out PPNS if it hasn't been done already.
  2. Rip out all existing Push code actually (not ParseInstallation, just all the push utils for registering). Stub classes may need to be available for a while to keep the manifest references working. Changes to the manifest have always caused customer support issues.
  3. Deprecate the senderID field in the manifest. To use FCM you already need googleservices.json which includes this. Parse's one-off attribute will on longer be read in the FCM so it should be dropped from documentation to avoid confusion.
  4. Add or reuse a BrodacastReceiver to receive the com.google.firebase.INSTANCE_ID_EVENT. If I recall the innards correctly, this may be a local broadcast event so it could possibly even be done dynamically. To avoid an SDK dependency on GMSCore you'll need to find out which extra had the InstanceId before you stuff it in the ParseInstallation. About 10LOC can replace hundreds of push code and without any GMSCore dependency.

The last bit I'm not clear about is what should be done with ParsePushBroadcastReceiver. It has a lot of cool features that I was proud of at the time, but it's largely redundant with FirebaseMessagingService now. For better or worse, its interpretation of the payload sets the standard. Parse developers will be able to use multiple push providers if they use a standard message payload.

ParseServer should probably have android-specific code (just like it has iOS specific code) to push the visible fields into the notification key. From there I'd recommend either promoting FirebaseMessagingService as as a single standard across Android or upgrade ParsePushBroadcastReceiver to understand notification in the payload.

@inlined
Copy link
Author

inlined commented Jan 16, 2017

CC @grantland, @daisuke-nomura and @wangmengyan95 who were auto-assigned from the last PR.

@rogerhu
Copy link
Contributor

rogerhu commented Mar 7, 2017

I agree with all 4 items. The Parse Push code is redundant. FCM also includes multiple google-services.json (https://developers.google.com/android/guides/google-services-plugin) which is important for testing against different product flavors.

Now that Parse has shutdown, can we move forward with stubbing out all of the Parse Push code as an interim step?

@rogerhu rogerhu mentioned this issue Mar 8, 2017
2 tasks
@rogerhu
Copy link
Contributor

rogerhu commented Mar 25, 2017

Related discussion here as well: #445

@Jawnnypoo
Copy link
Member

Jawnnypoo commented Apr 10, 2018

We've got to make a move on moving from GCM to FCM by the end of the year. Here is a screenshot of an official email from Google:

image

My recommendation would be to not go the current way of avoiding Play Services dependencies, and instead, make a Parse-SDK-Android-FCM repo which has the needed code and setup. This way, people can include and use the FCM support if they want/need it and are not forced into any unwanted dependencies.

@flovilmart
Copy link
Contributor

@Jawnnypoo is it something you’d wanna take the lead on?

@Jawnnypoo
Copy link
Member

I would be happy to. I think I can start with a module within this repo for now. Do you know if the server project will support FCM presently, or will changes need to be made server side too, @flovilmart ?

@rogerhu
Copy link
Contributor

rogerhu commented Apr 10, 2018

@natario1 had started some work, at least the cleanup (#732) sets us up fairly nicely.

https://developers.google.com/cloud-messaging/android/android-migrate-fcm

  1. Add InstanceID registration
  2. Migrate GcmListener

A lot of the steps are highlighted here: https://guides.codepath.com/android/Google-Cloud-Messaging#implement-a-registration-intent-service

We will need to modify the push-adapter as well to use the new GCM endpoints:

https://github.com/parse-community/parse-server-push-adapter/tree/master/src

@rogerhu
Copy link
Contributor

rogerhu commented Apr 10, 2018

@Jawnnypoo yeah a module will be fine in this repo will be fine for now. easier to iterate I think..

@rogerhu
Copy link
Contributor

rogerhu commented Apr 10, 2018

The node-gcm module, which the parse-server-push-adapter, is already using the fcm endpoints -- https://github.com/ToothlessGear/node-gcm/blob/master/lib/constants.js

So I think as long as we add the registration intent service, listener service, fix the receiver class (FCMMessageHandler), and figure out how to deal with conflicting permissions (the c2dm perms in GCM)...

@flovilmart
Copy link
Contributor

I believe everything is ok on the adapter side, but making sure both can coexist would be nice.

@Jawnnypoo
Copy link
Member

Okay, so progress update: I got this all working last night with FCM. It is in another repo so that I could test it on a test app that already had Firebase and a Parse server set up. It works great!

The problem is, it sends out two push notifications on the device, one from GCM, one from FCM. This is due to the fact that the FCM messaging gradle dependency automatically does all the manifest permissions and setup for FCM, which seems to be somewhat identical to GCM, which causes both GCM and FCM notifications to fire.

This means that we will basically have to rip GCM out of the core Parse Android SDK and make it it's own module, similar to what we are doing for FCM. The real downside to that it that there is really no way to do this in a backwards compatible way. We can make sure to make the migration path really clean and easy (probably would just be changing some import statements), but I don't see a clean way to move forward and not put GCM into its own module.

I am happy to take that on as well, I just wanted to make you guys aware, since it will probably require a 1.17.0 update and modifying a bit of documentation across the board.

@rogerhu
Copy link
Contributor

rogerhu commented Apr 11, 2018

Yep makes sense. We might need to put a warning if no push adapter is registered/detected on Parse.Init..

@Jawnnypoo
Copy link
Member

Looks like we can finally close the books on this one 🎉

@flovilmart
Copy link
Contributor

That’s really really awesome! Great job guys! 👏

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

No branches or pull requests

4 participants