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

adds support for getToken(string,string) and deleteToken() #1215

Merged
merged 2 commits into from
Jun 18, 2018

Conversation

ctaintor
Copy link
Contributor

@ctaintor ctaintor commented Jun 15, 2018

I've added support for deleteToken() as well as the other form of getToken(). This allows for someone to request or delete FCM tokens for sender IDs other than the default senderid. I also added some tests for instanceId methods - they don't do much but at least will break if someone broke the native implementations.

@chrisbianca
Copy link
Contributor

chrisbianca commented Jun 17, 2018

Many thanks for adding these methods, but could I ask that the new signatures are moved to the instanceId (iid) module? The messaging module aims to replicate the JS messaging SDK which doesn't support these advanced parameters.

We created the instanceId module to allow us to include functionality that's specific to instance ID on Android and iOS.

@ctaintor
Copy link
Contributor Author

This is what I originally did, but the getToken() was in messaging, hence why I added them to messaging and not iid.

@chrisbianca
Copy link
Contributor

Yes, there is unfortunately a bit of overlap, but for the majority of use cases, the simple getToken is all that developers will need. For these advanced methods, it makes sense to keep them separate in the instanceId module rather than complicate the messaging module further.

@ctaintor
Copy link
Contributor Author

Makes sense. Just to make sure, this will mean that getToken() will exist in two places. Cool?

@chrisbianca
Copy link
Contributor

Yep, the messaging module will have getToken() with no parameters, and instanceId will have the more complicated getToken() implementation. Thank you!

@ctaintor
Copy link
Contributor Author

OK - would you prefer the iid version to also support no parameters? Or have it only be the parameter version.

@chrisbianca
Copy link
Contributor

Let's go with only the parameter version.

@ctaintor
Copy link
Contributor Author

OK - I'm having trouble getting anything to build right now, but this should work, heh. Cocoapods...

@ctaintor
Copy link
Contributor Author

Have the tests been broken in master? I'm currently unable to install cocoapods:

[!] CocoaPods could not find compatible versions for pod "RNFirebase":
  In snapshot (Podfile.lock):
    RNFirebase (= 4.1.0)

  In Podfile:
    RNFirebase (from `../../ios/RNFirebase.podspec`)

It seems like you've changed the constraints of dependency `RNFirebase` inside your development pod `RNFirebase`.
You should run `pod update RNFirebase` to apply changes you've made.

(tried with a fresh clone of master and following steps here)

@ctaintor
Copy link
Contributor Author

OK - should work now.

@chrisbianca
Copy link
Contributor

This looks awesome, thank you for moving it over.

Just one last thing before I can merge it in - are you able to add the Typescript typings to lib/index.d.ts?

@chrisbianca chrisbianca merged commit d7d6124 into invertase:master Jun 18, 2018
@chrisbianca
Copy link
Contributor

Thank you again for getting this added.

@oblador
Copy link
Contributor

oblador commented Jun 25, 2018

@chrisbianca This is great and I need it too, mind creating a release for this?

@bilalsyed001
Copy link
Contributor

This is forcing FirebaseMessaging to be installed. Failing to compile with Core

@Salakar
Copy link
Member

Salakar commented Jul 9, 2018

@bilalsyed001 hmm you're right, can see this PR adds a FIRMessaging import + usage for the options param on getToken

@ctaintor is this definitely a requirement? If so then we should probably adjust the docs to state it's dependent on messaging.


Loving react-native-firebase and the support we provide? Please consider supporting us with any of the below:

@ctaintor
Copy link
Contributor Author

ctaintor commented Jul 9, 2018

You need to pass the apns token. You could change the native signature for getToken to accept the options and inject the apns token from elsewhere. This requires the token to be exposed in js I believe but would mean messaging is only required if you use getToken.

Another option is to move these to messaging.

I won't have time to do this for a while since I'm on vacation until the end of the month.

@ctaintor
Copy link
Contributor Author

ctaintor commented Jul 9, 2018

Or you can require messaging. getToken and deleteToken are tightly coupled anyway... but I can see some value in just wanting to use the iid token.

Salakar added a commit that referenced this pull request Jul 10, 2018
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

5 participants