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

feat(messaging): ios & android messaging updates & fixes #3339

Merged
merged 47 commits into from
Mar 27, 2020

Conversation

Ehesp
Copy link
Member

@Ehesp Ehesp commented Mar 23, 2020

This PR is a follow-on to the android only updates (#3309) now in 6.4.0-rc2.

Android & iOS

  • [FEAT] Messaging now supports onNotificationOpenedApp & also getInitialNotification APIs. These are used to detect if a user opened the app via pressing a notification.

  • [FEAT] The RemoteMessage API now includes a notification payload (if present) when being sent to the messaging APIs (e.g. onMessage).

Android

iOS

We've spent a massive amount of time on iOS over the last two weeks, figuring out the quirks and going over the main issues on the library. iOS messaging is just overall super confusing compared to Android, especially the initial setup with certificates etc. Hopefully this PR will help drastically 🙏

  • [FEAT] setBackgroundMessageHandler support for iOS!

  • [FEAT] The permissions API has been upgraded to now support custom permissions. The permission API selects sensible defaults however allows you to fully customise them if required. Please note, provisional permissions is now disabled by default (previously, it was enabled by default for iOS 12+ devices):

// Default permissions:
await messaging().requestPermission({
  alert: true,
  announcement: false,
  badge: true,
  carPlay: true,
  provisional: false,
  sound: true,
});
  • [FEAT] The requestPermission & hasPermission API now returns the current authorization status, rather than a boolean value. This generally won't break existing apps as any "truthy" logic checks should still work:
const permission = await messaging().requestPermission();
// or const permission = await messaging().hasPermission();

if (permission === IOSAuthorizationStatus.NOT_DETERMINED) {
  // -1 =  No permission has been requested yet
}

if (permission === IOSAuthorizationStatus.DENIED) {
  // 0 =  User declined permission. They must enable permissions from the app settings
}

if (permission === IOSAuthorizationStatus.AUTHORIZED) {
  // 1 =  User granted permission
}

if (permission === IOSAuthorizationStatus.PROVISIONAL) {
  // 2 =  User accepted provisional permission
}
  • [DEPRECATION] registerForRemoteNotifications has been deprecated in favour of registerDeviceForRemoteMessages. It will be removed in v7. Underlying functionality has not changed, just renamed to avoid confusion with messages vs notifications.

  • [DEPRECATION] isRegisteredForRemoteNotifications has been deprecated in favour of isDeviceRegisteredForRemoteMessages. It will be removed in v7. Underlying functionality has not changed, just renamed to avoid confusion with messages vs notifications.

  • [DEPRECATION] unregisterForRemoteNotifications has been deprecated in favour of unregisterDeviceForRemoteMessages. It will be removed in v7. Underlying functionality has not changed, just renamed to avoid confusion with messages vs notifications.

  • [FIX] Calling registerDeviceForRemoteMessages/registerForRemoteNotifications was causing permissions to be requested before explicitly requesting them via the messaging API.

  • [FIX] Fixed an issue whereby registering the device was not being called if it was already registered internally. Devices should always register when registerDeviceForRemoteMessages is called as per Apple guidelines, irregardless of current registration status. Make sure you always call registerDeviceForRemoteMessages during your app initialization on iOS.

  • [FIX] In cases where requesting an FCM with default scope/authorizedEntity, the underlying code now uses the recommended instanceIDWithHandler vs tokenWithAuthorizedEntity. This fixes an issue where FCM would throw a "The operation couldn’t be completed" error. Extension of Fix getToken() iOS function for messaging #3130.

  • [FIX] Direct FCM connection is now fixed. When the app was in the foreground, data-only messages were not coming through.

  • [FIX] When in debug mode, the APNS token will be set as a "sandbox" key type as per the Apple guidelines.

  • [FIX] The original APNS swizzling we implemented was not functioning correctly with application:didReceiveRemoteNotification:fetchCompletionHandler:. We added additional logic whereby this is executed in all scenarios (foreground/background/quit) and replaces a deprecated Apple API. This fixes issues with data-only messages not being handled by the device.

  • [FIX] When the application is in the foreground, notification messages were not being sent to the onMessage handler (via willPresentNotification). This is now fixed.

  • [FIX] FIRMessagingDelegate methods added to your AppDelegate.m will also be called (fixes 🔥 RNFirebaseMessaging is intercepting FCM delegate call #1814)

@codecov
Copy link

codecov bot commented Mar 23, 2020

Codecov Report

Merging #3339 into master will increase coverage by 4.09%.
The diff coverage is 20%.

@@            Coverage Diff             @@
##           master    #3339      +/-   ##
==========================================
+ Coverage   81.55%   85.64%   +4.09%     
==========================================
  Files         108      108              
  Lines        3409     3418       +9     
==========================================
+ Hits         2780     2927     +147     
+ Misses        629      491     -138

@vovka-s
Copy link

vovka-s commented Apr 10, 2020

On release 6.4.0 onNotificationOpenedApp is still not called when I press a notice on Android

@sndp07
Copy link

sndp07 commented Apr 12, 2020

I recently upgraded to 6.4.0. Messaging works fine in Android, and iOS simulator, but it doesn't work on the real device. Am I missing any configuration specific to iOS real device?

@LucasGarcez
Copy link
Contributor

onNotificationOpenedApp does't called when open a notification on Android. And getInitialNotification always return null in both platform.

Copy link
Contributor

@TPXP TPXP left a comment

Choose a reason for hiding this comment

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

I had another workaround on my side for the 1001 error code, and fetching an up-to-date notification token on iOS. I'm not sure tokens fetched with the new API will change if the APNS token changes for whatever reason (which is the reason we should always ask iOS for it).

Also, I'm not sure if deleteToken still works with a token from instanceIDwithHandler. On my side, we used https://firebase.google.com/docs/reference/ios/firebaseinstanceid/api/reference/Classes/FIRInstanceID#-deleteidwithhandler, which will destroy all tokens. That's potentially destroying more tokens than expected though, but do we know the scope of token from instanceIdWithHandler ?

}];
} else {
NSDictionary *options = nil;
if ([FIRMessaging messaging].APNSToken) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the APNSToken is not set (returning user), an 1001 error is likely to be thrown. Showing a warning message "did you forget to call registerForRemoteNotifications() ?" could help.

UNAuthorizationOptions authOptions;
if (@available(iOS 12.0, *)) {
authOptions = UNAuthorizationOptionProvisional | UNAuthorizationOptionAlert | UNAuthorizationOptionSound | UNAuthorizationOptionBadge;
if ([UIApplication sharedApplication].isRegisteredForRemoteNotifications == YES) {
Copy link
Contributor

@TPXP TPXP Apr 25, 2020

Choose a reason for hiding this comment

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

This condition is broken and is the real root cause of the 1001 error code.
Diving into the code of the iOS SDK reveals the 1001 code is kFIRInstanceIDErrorCodeMissingAPNSToken. This error code is thrown by the tokenWithAuthorizedEntity method if it cannot find an APNS token to sign the token with: https://github.com/firebase/firebase-ios-sdk/blob/c049f741f0d3b7de955dff7c4173baedf3aa8355/Firebase/InstanceID/FIRInstanceID.m#L296
This will occur if the app is opened again after registering for remote notifications previously as it will not register again, thus the didRegisterForRemoteNotificationsWithDeviceToken callback will not be called with the APNS token and the Firebase SDK will not have it.

While instanceIDWithHandler has a caching system that will auto-fill the APNS token, which is why #3130 worked, we should not bypass registering for notifications in any case since iOS will reply with an updated token if needed.
According to the official docs:

You register your app and receive your device token each time your app launches using Apple-provided APIs.

Also, registering if the app is already registered is perfectly fine by the docs:

Your app registers with APNs for remote notifications, as shown in the top arrow. If the app is already registered and the app-specific device token has not changed, the system quickly returns the existing token to the app and this process skips to step 4[didRegisterForRemoteNotificationsWithDeviceToken is called].

@15110011
Copy link

15110011 commented May 6, 2020

i used onMessage and its return data is an empty object. So, how do I fix that?

@nicpro85
Copy link

nicpro85 commented May 18, 2020

i used onMessage and its return data is an empty object. So, how do I fix that?

did you find out why ? I have this issue on android.

{"collapseKey": "ch.myapp", "data": {}, "from": "593590134815", "messageId": "0:1589793258145266%2e1fdf9e2e1fdf9e", "notification": {"android": {"imageUrl": "3877", "sound": "Default"}, "body": "my great title"}, "sentTime": 1589793258119, "ttl": 2419200}

@andersonaddo
Copy link
Contributor

Just putting it out there that the deprecated functions (like registerForRemoteNotifications) are still there :P
We're in v8 now haha

andersondanilo pushed a commit to vixtech/react-native-firebase that referenced this pull request Nov 9, 2020
)

See PR invertase#3339 for changes.

Co-authored-by: Salakar <mike.diarmid@gmail.com>
andersondanilo pushed a commit to vixtech/react-native-firebase that referenced this pull request Nov 9, 2020
)

See PR invertase#3339 for changes.

Co-authored-by: Salakar <mike.diarmid@gmail.com>
andersondanilo pushed a commit to vixtech/react-native-firebase that referenced this pull request Nov 9, 2020
)

See PR invertase#3339 for changes.

Co-authored-by: Salakar <mike.diarmid@gmail.com>
hmhm2292 pushed a commit to hmhm2292/react-native-firebase that referenced this pull request Jul 13, 2021
)

See PR invertase#3339 for changes.

Co-authored-by: Salakar <mike.diarmid@gmail.com>
androidIsForVivek pushed a commit to androidIsForVivek/react-native-firebase that referenced this pull request Aug 9, 2021
)

See PR invertase#3339 for changes.

Co-authored-by: Salakar <mike.diarmid@gmail.com>
androidIsForVivek pushed a commit to androidIsForVivek/react-native-firebase that referenced this pull request Sep 15, 2021
)

See PR invertase#3339 for changes.

Co-authored-by: Salakar <mike.diarmid@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform: android platform: ios plugin: messaging FCM only - ( messaging() ) - do not use for Notifications
Projects
None yet