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

Fix getToken() iOS function for messaging #3130

Closed
wants to merge 3 commits into from

Conversation

anfriis
Copy link

@anfriis anfriis commented Jan 24, 2020

This function often throws an exception:
Cloud Messaging "[messaging/unknown] The operation couldn’t be completed"
This fixes it and returns the FCM token successfully.

Fixes #2657

Summary

Checklist

  • Supports Android
  • Supports iOS
  • e2e tests added or updated in packages/**/e2e
  • Flow types updated
  • Typescript types updated

Test Plan

I have tested on two different iPhone's, trying to get FCM token several times using this function, and it works now whereas it didn't before because it threw the exception above.

Release Plan

[IOS][BUGFIX] [messaging] - Fix getToken() iOS function for messaging

[CATEGORY][type] [LOCATION] - Message


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

The function was returning `null` most of the time
@mikehardy
Copy link
Collaborator

Tagging in @Salakar and @Ehesp - this could be a fix for what appears to me to be the hottest issue in the repo right now

@mikehardy mikehardy requested review from Salakar and Ehesp January 24, 2020 13:47
@Salakar
Copy link
Member

Salakar commented Jan 24, 2020

Thanks for the PR, but this looks incomplete to me, authorisedEntity, scope & options args are no longer handled in the method (breaking change).

image

@Salakar Salakar added the blocked: do-not-merge Do not merge this issue without approval by the person who labelled this issue as Do Not Merge label Jan 24, 2020
@anfriis
Copy link
Author

anfriis commented Jan 24, 2020

Thanks for reviewing @Salakar.
It may be a breaking change for someone whom it is currently working for. Though I assume that very few people (perhaps nobody) are using the advanced usage params authorizedEntity and scope, which defaults to 'FCM'. and options. I am unfortunately not that deep into the internals of the framework to be able to change this.
By looking at the feedback on the issue #2657, the getToken() function is currently broken most of the time even for basic usage, so we should consider supporting basic usage first, and then support the more advanced usage later IMO.

@Ehesp
Copy link
Member

Ehesp commented Jan 25, 2020

@anfriis not sure that's a wise call on our end, we need it to be working with no breaking changes - who knows what people have done in their projects (patch packages, local fixes etc). We'll get it looked at asap.

@jacobp100
Copy link

What about if we call the function in this PR if the scope is undefined (defaulted to *) - and leave the current behaviour on other cases?

@MamyChow
Copy link

MamyChow commented Feb 6, 2020

Any update ?
I don't understand why we don't use instanceIDWithHandler because firebase's docs say that's better to use it unless there is a need to enable multiple senders.

@codecov
Copy link

codecov bot commented Feb 6, 2020

Codecov Report

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

@@           Coverage Diff           @@
##           master    #3130   +/-   ##
=======================================
  Coverage   89.77%   89.77%           
=======================================
  Files         109      109           
  Lines        3389     3389           
=======================================
  Hits         3042     3042           
  Misses        347      347

@ottob
Copy link

ottob commented Feb 7, 2020

This fixed getToken for me too. And then with #3075 I could finally get my notifications.

Thanks @anfriis!

@@ -96,11 +96,11 @@ - (NSDictionary *)constantsToExport {
options = @{@"apns_token": [FIRMessaging messaging].APNSToken};

Choose a reason for hiding this comment

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

You should also drop options, as they are not used anymore.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ Salakar
❌ Anders Friis


Anders Friis seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@Ehesp
Copy link
Member

Ehesp commented Mar 23, 2020

Thanks for sending this PR up. I'm going to close it as we ended up re-writing a lot of the iOS internals as part of wider fixes on #3339. We implemented this change along with supporting scopes/auth-entities too using the previous API.

@Ehesp Ehesp closed this Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked: do-not-merge Do not merge this issue without approval by the person who labelled this issue as Do Not Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

(FIXED) "[messaging/unknown] The operation couldn’t be completed".
9 participants