search

ostownsville / cordova-plugin-fcm

Google FCM Push Notifications Cordova Plugin
21 stars 17 forks source link

iOS apps are no longer subscribed to "all" by default #13

Closed remcohaszing closed 6 years ago

remcohaszing commented 6 years ago

The README states that apps are subscribed to all by default, but apparently an app I built using this fork, wasn't.

I suspect the changes in this fork are the cause, but I'm not 100% sure.

CowboyCode commented 6 years ago

@remcohaszing That should work still in Android as t I can see the code for it. But the ios, doesn't have the code anymore.

[[FIRMessaging messaging] subscribeToTopic:@"/topics/ios"]; [[FIRMessaging messaging] subscribeToTopic:@"/topics/all"];

@chrisjpalmer I have at the moment no ios device to test a fix. I guess, adding those two lines after init will do.

@remcohaszing If you want to test it, I have it in the fork below... https://github.com/CowboyCode/cordova-plugin-fcm

chrisjpalmer commented 6 years ago

Hi there everyone. Yes I was a bit iffy about this one. I removed it from the iOS side as I was unaware that it was in the readme and it was too difficult to implement.

The problem is, in order to subscribe to the topics immediately after the app starts, you need to wait for Firebase to become initialised. Otherwise an error is created saying that firebase is not initialised and the registration won't be successful. I tested this.

So logically one thinks, we need a callback for when Firebase has finished initialising. I think from memory there is one, however due to Firebase's 'method swizzling' it is inaccessible (Lookup method swizzling, its an objective-c thing). You can disable method swizzling, which then tasks the developer with the responsibility of calling the firebase API when APNS events occur. Its the kind of thing that would be easy to stuff up so I left it for the time being.

Ways forward:

  1. Forget the auto-subscription and remove it from the readme and android side
  2. Disable 'method swizzling' and re-add the iOS code for subscribing to topics.

Kind Regards Chris

CowboyCode commented 6 years ago

I would take it out, it's not a problem to subscribe manual. What is the point off auto subscribe anyway?

luigi37 commented 6 years ago

I would take it out, it's not a problem to subscribe manual. What is the point off auto subscribe anyway?

Well, in my case, I have an App where everybody is subscribed and provide its token along with some parameters. Server check certain (variable and unpredictable) conditions and if they meet the parameters, a notification get sent (only to the specific token). Would this be broken in case we remove the subscribe to all?

remcohaszing commented 6 years ago

What is the goal of this fork?

There's a PR from this repository, called "Firebase IOS SDK upgrade to 4.1.0".

This seems to fix a lot compared to the original repository.

However, there seem to be changes that aren't related to the PR description.

Also there seems to be very little activity on the original repository.

So to get back to whether or not to subscribe on the "all" and "ios" topics:

If the goal of this fork is to fulfill the purpose of upgrading Firebase, then yes, this should be fixed.

If the goal of this fork is to become a replacement for the original repository, then continue the discussion on whether or not an auto subscribe is a good idea. Then it would also be a good idea to keep this consistent for Android, document the state of this project, and publish this to npm.

chrisjpalmer commented 6 years ago

Hi @remcohaszing. Originally we wanted to merge with the owner of cordova-plugin-fcm. We had some email contact with the owner but they have not given us the ability to merge with their repo. We are a bit stranded.

However, I think it would be best that we part ways with the original owner and do as you say, document the project state and publish it to npm.

My opinion is that for now, lets remove this auto subscription since it wasn't even documented in the original plugin => how would people have known that it did this?

What are your thoughts @CowboyCode ?

CowboyCode commented 6 years ago

I agree with Chris, I have many apps using the plugin, so I need it running.

remcohaszing commented 6 years ago

@chrisjpalmer That would be great. 😄

One side note; the auto subscription is documented under https://github.com/ostownsville/cordova-plugin-fcm#subscribe-to-topic in the comment in the JS code block.

chrisjpalmer commented 6 years ago

Okay brilliant. Thanks for that piece of information. Will update the readme

chrisjpalmer commented 6 years ago

I am going to close this issue now as we have documented it we are no longer supporting auto subscription.