Skip to content

Commit

Permalink
ios notifications: Stop using react-native-notifications.
Browse files Browse the repository at this point in the history
Some (rather rude! yet still disregarded and closed-as-stale)
comments [1] on wix/react-native-notifications report that it's a
shame to see a decrease in activity from the maintainers, since it's
the only viable cross-platform library for RN push notifications.

But we don't even use it in a cross-platform way; we uprooted the
last of the linking logic on Android in 01b33ad. It also seems
strange to have two different libraries doing work to support push
notifications on iOS. See also discussion [2].

So, let PushNotificationIOS from react-native take responsibility
for what this library has been doing. Then, since *that's*
deprecated, an upcoming commit will have us using that same code but
from react-native-community, where it's maintained.

These were my steps:

1. Use the setup instructions for our version of
   `react-native-notifications` [3] to tear it down.

2. Use the setup instructions for `PushNotificationIOS` [4] from RN
   v0.60 to complete our setup. Some parts were missing, whether
   because we didn't need that functionality before, or it wasn't
   available in earlier RN versions.

3. Change the bit under the comment "Called when a notification is
   delivered to a foreground app." to correctly call the completion
   handler with UNNotificationPresentationOptions instead of
   UNAuthorizationOptions. N.B.: Also, remove the options that ask
   it to play a sound and show an alert, as these aren't part of the
   current behavior. That just leaves updating the badge count.

4. Make tiny, NFC adjustments, mostly to indentation, to smooth the
   transition to the react-native-community module.

5. Change the call sites to use PushNotificationIOS, and update some
   types and comments. One part that stands out is the removal of
   the "consumeBackgroundQueue" hack from c0e2233. Nothing further
   is necessary, it just works. :)

6. Do various housekeeping things like removing the libdef.

7. On iOS, test that notifications still appear in the closed and
   background states and that, from either state, they navigate to
   the corresponding narrow. All works as expected, with one
   "gotcha": from a cold start, in debug mode, sometimes
   notifications don't navigate. There's an open issue for this [5],
   and it seems it doesn't affect release builds. In debug mode, I
   was able to solve it by disabling "Debug JS Remotely", following
   one comment there. In any case, `getInitialNotification` is what
   we've already been using PushNotificationIOS for, for a long
   time, so this hiccup is not new.

8. On Android, test that notifications appear (regardless of
   closed/background/foreground state) and navigate to the
   corresponding narrow.

[1]: wix/react-native-notifications#519 (comment)
[2]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.20push.20notifications/near/825122
[3]: https://github.com/wix/react-native-notifications/blob/882775fb5/docs/installation.md
[4]: https://reactnative.dev/docs/0.60/pushnotificationios
[5]: react-native-push-notification/ios#24 (comment)
  • Loading branch information
chrisbobbe committed Nov 10, 2020
1 parent 08ea7d1 commit 3ce528e
Show file tree
Hide file tree
Showing 11 changed files with 113 additions and 158 deletions.
1 change: 0 additions & 1 deletion .flowconfig
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ node_modules/warning/.*
# resolved, or write our own libdef.
.*/node_modules/react-native-image-picker/.*

.*/node_modules/react-native-notifications/.*
.*/node_modules/react-native-tab-view/.*
.*/node_modules/react-native-safe-area/.*
.*/node_modules/flow-coverage-report/.*
Expand Down
53 changes: 0 additions & 53 deletions flow-typed/npm/react-native-notifications_vx.x.x.js

This file was deleted.

6 changes: 0 additions & 6 deletions ios/Podfile.lock
Original file line number Diff line number Diff line change
Expand Up @@ -270,8 +270,6 @@ PODS:
- React
- react-native-netinfo (5.9.5):
- React
- react-native-notifications (1.5.0):
- React
- react-native-photo-view (1.5.2):
- React
- react-native-safari-view (1.0.0):
Expand Down Expand Up @@ -443,7 +441,6 @@ DEPENDENCIES:
- "react-native-cameraroll (from `../node_modules/@react-native-community/cameraroll`)"
- react-native-image-picker (from `../node_modules/react-native-image-picker`)
- "react-native-netinfo (from `../node_modules/@react-native-community/netinfo`)"
- react-native-notifications (from `../node_modules/react-native-notifications`)
- react-native-photo-view (from `../node_modules/react-native-photo-view`)
- react-native-safari-view (from `../node_modules/react-native-safari-view`)
- react-native-safe-area (from `../node_modules/react-native-safe-area`)
Expand Down Expand Up @@ -554,8 +551,6 @@ EXTERNAL SOURCES:
:path: "../node_modules/react-native-image-picker"
react-native-netinfo:
:path: "../node_modules/@react-native-community/netinfo"
react-native-notifications:
:path: "../node_modules/react-native-notifications"
react-native-photo-view:
:path: "../node_modules/react-native-photo-view"
react-native-safari-view:
Expand Down Expand Up @@ -674,7 +669,6 @@ SPEC CHECKSUMS:
react-native-cameraroll: 06e60780a4e6e7bb9a588eca72506744cf6e133b
react-native-image-picker: 3d3f85baabca60a00b75fb8facc1376db7bbdafa
react-native-netinfo: a53b00d949b6456913aaf507d9dba90c4008c611
react-native-notifications: ce37363008fe2d6a226da4e721eace23b6ae3ad9
react-native-photo-view: 63e9e61da873531f931008b545d8d10c5373ddf8
react-native-safari-view: 955d7160d159241b8e9395d12d10ea0ef863dcdd
react-native-safe-area: 5fce5242419932bc05656f31bc5f0716e30be0f6
Expand Down
1 change: 0 additions & 1 deletion ios/ZulipMobile-Bridging-Header.h
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,3 @@
#import <React/RCTEventEmitter.h>
#import <React/RCTBundleURLProvider.h>
#import <React/RCTPushNotificationManager.h>
#import <RNNotifications.h>
31 changes: 24 additions & 7 deletions ios/ZulipMobile/AppDelegate.m
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
#import <React/RCTLinkingManager.h>
#import <asl.h>
#import <React/RCTLog.h>
#import <RNNotifications.h>
#import <UserNotifications/UserNotifications.h>
#import <React/RCTPushNotificationManager.h>
#import <UMCore/UMModuleRegistry.h>
#import <UMReactNativeAdapter/UMNativeModulesProxy.h>
Expand Down Expand Up @@ -57,6 +57,11 @@ - (BOOL)application:(UIApplication *)application didFinishLaunchingWithOptions:(
rootViewController.view = rootView;
self.window.rootViewController = rootViewController;
[self.window makeKeyAndVisible];

// Define UNUserNotificationCenter
UNUserNotificationCenter *center = [UNUserNotificationCenter currentNotificationCenter];
center.delegate = self;

return YES;
}

Expand Down Expand Up @@ -94,30 +99,42 @@ - (BOOL)application:(UIApplication *)application continueUserActivity:(NSUserAct
// Required to register for notifications
- (void)application:(UIApplication *)application didRegisterUserNotificationSettings:(UIUserNotificationSettings *)notificationSettings
{
[RNNotifications didRegisterUserNotificationSettings:notificationSettings];
[RCTPushNotificationManager didRegisterUserNotificationSettings:notificationSettings];
}

// Required for the register event.
- (void)application:(UIApplication *)application didRegisterForRemoteNotificationsWithDeviceToken:(NSData *)deviceToken
{
[RNNotifications didRegisterForRemoteNotificationsWithDeviceToken:deviceToken];
[RCTPushNotificationManager didRegisterForRemoteNotificationsWithDeviceToken:deviceToken];
}

// Required for the registrationError event.
- (void)application:(UIApplication *)application didFailToRegisterForRemoteNotificationsWithError:(NSError *)error {
[RNNotifications didFailToRegisterForRemoteNotificationsWithError:error];
[RCTPushNotificationManager didFailToRegisterForRemoteNotificationsWithError:error];
}

// Required for the notification event.
// Required for the notification event. You must call the completion handler after handling the remote notification.
- (void)application:(UIApplication *)application didReceiveRemoteNotification:(NSDictionary *)userInfo
fetchCompletionHandler:(void (^)(UIBackgroundFetchResult))completionHandler
{
[RNNotifications didReceiveRemoteNotification:userInfo];
[RCTPushNotificationManager didReceiveRemoteNotification:userInfo fetchCompletionHandler:completionHandler];
}

// Required for the localNotification event.
- (void)application:(UIApplication *)application didReceiveLocalNotification:(UILocalNotification *)notification
{
[RNNotifications didReceiveLocalNotification:notification];
[RCTPushNotificationManager didReceiveLocalNotification:notification];
}

// Called when a notification is delivered to a foreground app.
-(void)userNotificationCenter:(UNUserNotificationCenter *)center
willPresentNotification:(UNNotification *)notification
withCompletionHandler:(void (^)(UNNotificationPresentationOptions options))completionHandler
{
// Update the badge count. Do not play sound or show an alert. For
// these options see
// https://developer.apple.com/documentation/usernotifications/unnotificationpresentationoptions?language=objc
completionHandler(UNNotificationPresentationOptionBadge);
}

@end
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@
"react-native-document-picker": "^3.2.4",
"react-native-gesture-handler": "^1.0.12",
"react-native-image-picker": "^2.3.3",
"react-native-notifications": "^1.2.0",
"react-native-photo-view": "alwx/react-native-photo-view#c58fd6b30",
"react-native-reanimated": "^1.0.0",
"react-native-safari-view": "2.0.0",
Expand Down
6 changes: 0 additions & 6 deletions react-native.config.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,12 +22,6 @@ module.exports = {
ios: null,
},
},
'react-native-notifications': {
platforms: {
// We don't use this Wix library in the Android build. See 01b33ad31.
android: null,
},
},
'react-native-screens': {
platforms: {
// We haven't enabled `react-native-screens` yet, that's
Expand Down
9 changes: 1 addition & 8 deletions src/boot/AppEventHandlers.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,7 @@ import type { Dispatch, Orientation as OrientationT } from '../types';
import { createStyleSheet } from '../styles';
import { connect } from '../react-redux';
import { getUnreadByHuddlesMentionsAndPMs } from '../selectors';
import {
handleInitialNotification,
NotificationListener,
notificationOnAppActive,
} from '../notification';
import { handleInitialNotification, NotificationListener } from '../notification';
import { ShareReceivedListener, handleInitialShare } from '../sharing';
import { appOnline, appOrientation, initSafeAreaInsets } from '../actions';
import PresenceHeartbeat from '../presence/PresenceHeartbeat';
Expand Down Expand Up @@ -109,9 +105,6 @@ class AppEventHandlers extends PureComponent<Props> {
if (state === 'background' && Platform.OS === 'android') {
NativeModules.BadgeCountUpdaterModule.setBadgeCount(unreadCount);
}
if (state === 'active') {
notificationOnAppActive();
}
};

notificationListener = new NotificationListener(this.props.dispatch);
Expand Down
6 changes: 3 additions & 3 deletions src/notification/extract.js
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,9 @@ export const fromAPNsImpl = (rawData: JSONableDict): Notification | void => {
// required, with a structure defined by Apple; all other entries are
// available to the application.
//
// The react-native-notifications library filters out `aps`, parses it, and
// hands us the rest as "data". Pretty much any iOS notifications library
// should do the same, but we don't rely on that.
// PushNotificationsIOS filters out `aps`, parses it, and hands us the rest
// as "data". Pretty much any iOS notifications library should do
// the same, but we don't rely on that.

const data: JSONableInputDict = (() => {
if ('aps' in rawData) {
Expand Down
Loading

0 comments on commit 3ce528e

Please sign in to comment.