Skip to content

Commit

Permalink
ios notifications: Stop using wix/react-native-notifications.
Browse files Browse the repository at this point in the history
wix/react-native-notifications doesn't seem to be maintained [1].
expo-notifications also works on both platforms, but we've decided
it's not for us [2].

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

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
   wix/react-native-notifications [4] to tear it down.

2. Use the setup instructions for `PushNotificationIOS` [5] from RN
   v0.62 to complete our setup. We hadn't yet done everything in
   these instructions, whether because we didn't need that
   functionality before, or it wasn't available in earlier RN
   versions. We did some of this setup in the previous commit.

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

4. 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. :)

5. Do various housekeeping things like removing the libdef.

6. 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 [6],
   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.

7. On Android, test that notifications appear (regardless of
   closed/background/foreground state) and that they 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/1061130
[3]: https://chat.zulip.org/#narrow/stream/243-mobile-team/topic/iOS.20push.20notifications/near/825122
[4]: https://github.com/wix/react-native-notifications/blob/882775fb5/docs/installation.md
[5]: https://reactnative.dev/docs/0.62/pushnotificationios
[6]: react-native-push-notification/ios#24 (comment)
  • Loading branch information
chrisbobbe committed Dec 2, 2020
1 parent 7a622ac commit 782c7f6
Show file tree
Hide file tree
Showing 11 changed files with 101 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>
14 changes: 7 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 @@ -99,30 +99,30 @@ - (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];
}

Expand Down
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 782c7f6

Please sign in to comment.