Start using @react-native-community/push-notification-ios #4163

chrisbobbe · 2020-06-17T01:42:57Z

And remove our dependency on wix/react-native-notifications.

I took this up before I saw Greg's comment here about zulip/zulip#15179:

I think this issue is actually a lot simpler than that. The PR will cause the server to start sending the information, and it's doing it in a way that iOS should already respond to even without any client-side code of ours.

Ah, well; that particular need doesn't seem to require any code changes on mobile. But I've been really wanting to get this done because

(1) I've often been kind of confused by our dependency on two different libraries for different iOS push notification needs, when there's no obvious reason why everything couldn't be done by one library, and
(2) some peace and clarity comes from being on the latest versions of things. PushNotificationIOS from RN core is deprecated, and there are complaints about how well the Wix library is maintained; see the issue, and discussion linked from there.

So I went ahead and put this together.

chrisbobbe · 2020-06-17T01:45:11Z

ios/ZulipMobile/AppDelegate.m

+      willPresentNotification:(UNNotification *)notification
+        withCompletionHandler:(void (^)(UNNotificationPresentationOptions options))completionHandler
+{
+  // Update the badge count. Do not play sound or show an alert. For


Here, we explicitly tell it to update the badge count if a notification is received while the app is in the foreground. I haven't tested the previous behavior, but it seems quite plausible that no badge count updates were being done, just as no alerts were being shown. So this seems helpful with zulip/zulip#15179 in mind.

I've kept the suppression of the alerts (and the sound) to mimic current behavior; it may be considered distracting to see notifications for an app that's already foregrounded.

I've just tested on 26.28.151 (from TestFlight) on CZO. Indeed, I can send myself messages from a test account (increasing the unread count) and go back to the home screen and see that the badge count did not change. Then I send myself another message, while the app is in the background, and the notification appears and the badge count updates. (It jumps to 609...but that's a server-side issue being investigated).

Being foregrounded should not suppress badge count updates; this PR should fix this. 🙂

Being foregrounded should not suppress badge count updates; this PR should fix this.

Hmm! Does that mean this fixes #4182? It sounds like it probably does.

This would probably be good to do as a separate, previous commit. It's described in that RN module's docs... but it's purely using upstream iOS APIs. So it doesn't actually depend on whether we're using that RN module or some other library, and we can just treat that bit of docs as a helpful tip about how to handle notifications in an iOS app.

gnprice

This looks like great work! Very happy to have this cleaned up.

I still need to read the last two commits (the main ones) more closely while looking at the upstream setup instructions. Below are some comments from just a quick read.

src/notification/index.js

gnprice · 2020-06-17T20:53:04Z

src/notification/index.js

    //   https://developer.apple.com/documentation/uikit/uiapplication/1622932-registerusernotificationsettings
    // (deprecated after iOS 10, yikes!); which after possibly prompting the
-    // user causes "the app" (i.e. the platform part) to call this, I think,
-    // though I haven't successfully traced all the steps there:


Does this mean you have traced through those steps? 🙂

I did, but I don't remember offhand what they all were, and there's a chance that I missed something—I'll come back to this and write up the steps.

In my current revision, these comments contain more details, and I'm more sure about them.

src/notification/index.js

chrisbobbe · 2020-06-30T01:22:37Z

Hmm, for the big picture, I've actually just remembered that Expo has this whole thing set up that we may (or more likely may not) want to buy into, where you can send push notifications through Expo's servers: https://github.com/expo/expo/tree/master/packages/expo-notifications#add-your-projects-credentials-to-expo-server-optional

Apparently expo-notifications is a thing, and you can install it just like any other Unimodule, without having to be in an Expo-"managed" project. The setup with Expo's servers is a completely optional part of this. This didn't occur to me to check when I followed the deprecation notice at https://reactnative.dev/docs/0.60/pushnotificationios, oops.

chrisbobbe · 2020-07-02T01:13:22Z

Pinging @gnprice on the above (#4163 (comment)) as it should have occurred to me earlier and it may be worth being aware of in some recent conversations about notifications (here maybe?).

chrisbobbe · 2020-09-16T00:26:49Z

OK, just fixed conflicts and adjusted some comments. I also filed react-native-push-notification/ios#179 for something I noticed in the library.

In particular, I was glad to see this time around that react-native-push-notification/ios#66 helped the project move away from methods that were deprecated in iOS 10.

chrisbobbe · 2020-09-16T18:30:30Z

src/notification/index.js

-    // event names differ!  wix has s/Error/Failed/ vs. upstream; also
-    // upstream has singular for failure although plural for success, ouch.)
+    // This leads to a call in RNCPushNotificationIOS to this, to
+    // maybe prompt the user to grant authorization, with options for


The Apple documentation for these newer things seems to prefer the term "authorization" over "permissions"; might as well echo that.

chrisbobbe · 2020-11-10T01:46:08Z

I've just fixed some conflicts, and updated a few issues having both the a-iOS and a-notifications labels (seen in GitHub's UI, just above this comment) as blocked on this work. It would be best to settle what packages we use for notifications on iOS (as this does) before fixing issues with notifications on iOS, at least if they're not p0 issues.

(@gnprice)

gnprice

Thanks @chrisbobbe !

We discussed in chat, and I think we want to stick with this line of libraries -- upgrading to the react-native-community library as this PR does -- rather than switch to the Expo notifications library.

Comments below -- all small, I think.

ios/ZulipMobile/AppDelegate.m

gnprice · 2020-11-10T21:33:40Z

ios/ZulipMobile/AppDelegate.m

+      willPresentNotification:(UNNotification *)notification
+        withCompletionHandler:(void (^)(UNNotificationPresentationOptions options))completionHandler
+{
+  // Update the badge count. Do not play sound or show an alert. For


Being foregrounded should not suppress badge count updates; this PR should fix this.

Hmm! Does that mean this fixes #4182? It sounds like it probably does.

gnprice · 2020-11-10T21:37:05Z

ios/ZulipMobile/AppDelegate.m

+      willPresentNotification:(UNNotification *)notification
+        withCompletionHandler:(void (^)(UNNotificationPresentationOptions options))completionHandler
+{
+  // Update the badge count. Do not play sound or show an alert. For


This would probably be good to do as a separate, previous commit. It's described in that RN module's docs... but it's purely using upstream iOS APIs. So it doesn't actually depend on whether we're using that RN module or some other library, and we can just treat that bit of docs as a helpful tip about how to handle notifications in an iOS app.

src/notification/index.js

ios/ZulipMobile/AppDelegate.h

ios/ZulipMobile/AppDelegate.m

gnprice · 2020-11-10T22:58:54Z

src/notification/index.js

@@ -267,53 +268,46 @@ export class NotificationListener {
 export const getNotificationToken = () => {
  if (Platform.OS === 'ios') {
    // This leads to a call in RNCPushNotificationIOS to this, to
-    // maybe prompt the user to grant permissions:
-    //   https://developer.apple.com/documentation/uikit/uiapplication/1622932-registerusernotificationsettings?language=objc
-    // (deprecated after iOS 10, yikes!).


This is one bit I'm particularly glad to be deleting 🙂

chrisbobbe · 2020-11-24T00:49:00Z

Being foregrounded should not suppress badge count updates; this PR should fix this.

Hmm! Does that mean this fixes #4182? It sounds like it probably does.

It's necessary but not sufficient, I think; we'll have to turn on sending badge counts other than zero, IIRC, and play with that for a bit. I seem to remember we ran into some additional issues when we temporarily turned that on.

chrisbobbe · 2020-12-02T00:17:51Z

(Still working on this; I'll post back here when it's ready for another review.)

This is based on the recommended setup for PushNotificationIOS from React Native [1], so we might have included it in the next commit, where we fully set that up. But as Greg points out [2], this code is purely using upstream iOS APIs, so it doesn't depend on whether we're using that RN module or something else, so it's fine to do here on its own. In a departure from those instructions, we decide to just pass the thing (`UNNotificationPresentationOptionBadge`) that updates the badge count when the app is foregrounded. This is a necessary part of zulip#4182, but one thing we'll still need to do for that is test that the server is sending the right badge counts to the client. [1] https://reactnative.dev/docs/0.62/pushnotificationios. These instructions are actually missing a required change to the AppDelegate.h, which we take here, having seen it in the instructions for @react-native-community/push-notification-ios (at https://github.com/react-native-push-notification-ios/push-notification-ios#update-appdelegateh). [2] zulip#4163 (comment)

chrisbobbe · 2020-12-02T20:06:43Z

OK, I've pushed another revision for review. 🙂 Please also see my comments at #4163 (comment) and #4163 (comment).

In this commit: - Update "react-native", "react", and "flow-bin" in package.json. Run `pod update Folly --no-repo-update` to move past an error on the `pod install` step of the `postinstall` script [1]. After this step, the Podfile.lock file is much changed, as expected. There is also a change in the project.pbxproj file. - Update .flowconfig with the new Flow version, and address one new warning by removing a `$FlowFixMe` that we shouldn't have needed in the first place. (Usually this part is much more complicated!) - Update "jest-expo" in package.json, to oblige a peer-dependency constraint on "react". - TODO: Do something about react-native-cameraroll. - Make a change to the Podfile, according to facebook/react-native@a35efb940. - Instead of spelling out all the Pods from RN (all the non-Flipper-related Pods, that is -- we'll handle the Flipper ones in a later commit), call a function defined by React Native, `use_react_native!`, newly exposed in RN v0.63. - Since we're still using React-RCTPushNotification (pending the resolution of zulip#4163), take care to keep React-RCTPushNotification. It apparently hasn't yet been removed from React Native, but it understandably isn't handled by `use_react_native!`. See the PR thread for the list of RN commits affecting the template app and how we've chosen to propagate the template-app changes into our project. See also the RN v0.62 -> v0.63 upgrade guide [2], which gives a visual representation of the changes to the template app. It mostly matches the changes we've made; important deviations should have been explained in the commit list in the PR thread [3]. [1] TODO: Link to explanation on PR. [2] https://react-native-community.github.io/upgrade-helper/?from=0.62.2&to=0.63.4 [3] As always, the guide does show some changes that don't appear in the template app. I think this noise is an effect of how the guide is generated (with react-native-community/rn-diff-purge) and can safely be ignored. It's a diff between a fresh app created with `react-native init --version=$CURRENT` and a fresh app created with `react-native init --version=$NEXT`, for the selected values of `$CURRENT` and `$NEXT`. In particular: - I believe that some dependency version range changes in package.json, including for @babel/core and @babel/runtime, might be caused by different versions for those dependencies being available when the `react-native init` commands are run. - Some changes in ordering and unique ID values always seem to show up in the project.pbxproj file.

This is based on the recommended setup for PushNotificationIOS from React Native [1], so we might have included it in the next commit, where we fully set that up. But as Greg points out [2], this code is purely using upstream iOS APIs, so it doesn't depend on whether we're using that RN module or something else, so it's fine to do here on its own. In a departure from those instructions, we decide to just pass the thing (`UNNotificationPresentationOptionBadge`) that updates the badge count when the app is foregrounded. This is a necessary part of zulip#4182, but one thing we'll still need to do for that is test that the server is sending the right badge counts to the client. [1] https://reactnative.dev/docs/0.62/pushnotificationios. These instructions are actually missing a required change to the AppDelegate.h, which we take here, having seen it in the instructions for @react-native-community/push-notification-ios (at https://github.com/react-native-push-notification-ios/push-notification-ios#update-appdelegateh). [2] zulip#4163 (comment)

chrisbobbe · 2021-05-10T17:41:00Z

(Just rebased and resolved some conflicts.)

gnprice

Thanks and sorry for the delay!

Small comments below.

The one remaining item that's more significant is: I've lost track a bit of what the state is of badge counts. It seems like this PR will have us start applying badge-count updates more. In a subthread last year #4163 (comment) , you mentioned testing the change and seeing the badge counts update:

I've just tested on 26.28.151 (from TestFlight) on CZO. Indeed, I can send myself messages from a test account (increasing the unread count) and go back to the home screen and see that the badge count did not change. Then I send myself another message, while the app is in the background, and the notification appears and the badge count updates. (It jumps to 609...but that's a server-side issue being investigated).

Being foregrounded should not suppress badge count updates; this PR should fix this.

But then in that linked chat thread, it looks like a few weeks later we turned off sending badge counts other than zero, at least for iOS.

What's your understanding of the current state of what this will do with badge counts? With current server behavior, will badge counts continue to not appear?

As long as that's the case -- really, as long as this won't make current behavior any worse -- I'm happy merging this and then proceeding to get badge counts working as a separate project.

gnprice · 2021-06-16T00:27:55Z

ios/ZulipMobile/AppDelegate.m

+// Required for localNotification event
+- (void)userNotificationCenter:(UNUserNotificationCenter *)center
+didReceiveNotificationResponse:(UNNotificationResponse *)response


Hmm, is this comment still right? (The method was application:didReceiveLocalNotification:, which more evidently matches this description.)

Hmm, good question. It's what the doc says, in the version we take in this commit: https://github.com/react-native-push-notification-ios/push-notification-ios/blob/v1.5.0/README.md

Also on master (as of posting this): https://github.com/react-native-push-notification-ios/push-notification-ios/blob/master/README.md

Looks like it was added in react-native-push-notification/ios#104, which was a PR about local notifications.

Ah, and it looks like it was added specifically to bring local-notification functionality to iOS 10+. From the PR description:

Local notification event should work on IOS 10+ with new didReceiveNotificationResponse and prior to IOS 10 using old didReceiveLocalNotification method.

So I would default to keeping this.

Cool, thanks for the investigation.

I still kind of have a suspicion that that wording is just cloned from the old version -- the diff in the PR looks like this:

// Required for the localNotification event.

→

// IOS 10+ Required for localNotification event - (void)userNotificationCenter:(UNUserNotificationCenter *)center didReceiveNotificationResponse:(UNNotificationResponse *)response withCompletionHandler:(void (^)(void))completionHandler { [RNCPushNotificationIOS didReceiveNotificationResponse:response]; completionHandler(); } // IOS 4-10 Required for the localNotification event.

But 🤷 that's good enough. In a future where we're writing more code of our own here, we'll spend the time to understand this platform API more deeply ourselves, and then we can write our own comment (either more accurate, or just more convincingly accurate, e.g. with a link to docs.)

src/notification/index.js

chrisbobbe · 2021-06-17T02:16:41Z

badge counts

Good thought to check in about that. I don't expect this PR to change any badge-count behavior for current servers.

That's because I believe current servers send a badge count of 0 with all notifications. (So it would be surprising if we started seeing badges, which only show if the badge count is above zero.) This is corroborated by these observations:

Without changes to mobile-app code, we were able to see badges for the brief period you mention, when we knew CZO was sending nonzero badge counts.
Also without changing mobile-app code, we stopped seeing badges when CZO stopped sending badge counts in zulip/zulip@2f66c825a.
On current master, the get_apns_badge_count_future from zulip/zulip@2f66c825a still exists and isn't used except in tests, and get_apns_badge_count still returns zero.

Re: this PR, it seems that iOS delegates control of badge counts to an app's code when the app is foregrounded. The commit ios notifications: Update the badge count if the app is foregrounded. accepts that responsibility by updating the badge count according to the notification's badge-count value, instead of not doing so. (And it declines to do other things like play a sound or show the notification banner.) I don't think there's much room for this code to set a wrong badge-count value, if the right value is in the notification; UNNotificationPresentationOptionBadge is what you say when you want to "[a]pply the notification's badge value to the app’s icon".

In chat, you've mentioned that you think servers might not be sending totally the right shape of thing to convey the badge count. I haven't yet been able to follow the reasoning there; do you still think that might be the case? 🙂 I think you may have also mentioned (on a video call?) that we might have a bug in client-side code for parsing the badge-count value; I don't think I've followed that either, and possibly I'm mis-remembering what you've said.

I do think there may have been a server bug where things in the database didn't look right; here's a message on CZO I'm looking at for that.

I know of one bug that's indicated by Tim's comment, "likely mobile app side issue with handling notifications while the app is open" from zulip/zulip@2f66c825a. It's the one we hope to fix in the badge-count commit in this PR. 🙂

This is based on the recommended setup for PushNotificationIOS from React Native [1], so we might have included it in the next commit, where we fully set that up. But as Greg points out [2], this code is purely using upstream iOS APIs, so it doesn't depend on whether we're using that RN module or something else, so it's fine to do here on its own. In a departure from those instructions, we decide to just pass the thing (`UNNotificationPresentationOptionBadge`) that updates the badge count when the app is foregrounded, and not ask for other things like showing the banner or playing a sound. (This new behavior is necessary for fixing zulip#4182, but I don't think it's sufficient: we'll still want to test that the server is sending the right badge counts to the client.) [1] https://reactnative.dev/docs/0.62/pushnotificationios. These instructions are actually missing a required change to the AppDelegate.h, which we take here, having seen it in the instructions for @react-native-community/push-notification-ios (at https://github.com/react-native-push-notification-ios/push-notification-ios#update-appdelegateh). [2] zulip#4163 (comment)

…ingly. As Greg says [1], "It actually seems hard to me to 100% confirm this from the implementation! [...] It might be simpler to appeal to what this method's job is: it's giving us the notification data, which was passed over APNs as JSON." So, do that. [1] zulip#4163 (comment)

As Greg says [1], > Probably "interesting" wasn't a very clear way to write that > description; the distinction that I think it's intended to get at > is "unexpected", as in "buggy". [1] zulip#4163 (comment)

And throw an error if it gets one, since it definitely indicates a bug. See discussion at zulip#4163 (comment).

chrisbobbe · 2021-06-17T02:20:50Z

Thanks for the review! Revision pushed. 🙂

gnprice · 2021-06-18T04:19:00Z

Cool, that's all very helpful, thanks. I'll go ahead and merge.

In chat, you've mentioned that you think servers might not be sending totally the right shape of thing to convey the badge count. I haven't yet been able to follow the reasoning there; do you still think that might be the case?

If we were to swap get_apns_badge_count_future back in on the server, I think what it would start sending still might not be right.

But as long as it's hard-coding a badge count of 0, any issues there are still dormant. So we're good to get this PR in, and then consider the next steps there.

There were a few different bugs going on in the investigation in that chat thread:

A bug where the server thought the right value for the badge count was something like 2, when it should be 0. That's the one discussed where you link to here:

I do think there may have been a server bug where things in the database didn't look right; here's a message on CZO I'm looking at for that.

I think that one is all cleared up; see that thread at 2020-07-05.
Server never sends notification, if recipient recently used webapp zulip#18072, the "Server never sends notification, if recipient recently used webapp" bug. That's long before I filed that particular issue thread, but this message is a clear signature of that bug:

Hmm but worse than the summary of that issue [meaning Never get notifications (when recently used webapp, or app is in background) #3659] suggests -- I had a test message at T + 5 minutes, and it never produced a notification
This bug: we weren't applying the badge-count update, on the client, when the app was in the foreground.
Some other bug somewhere, which results in the badge count being stuck at 1 once everything's read. Detailed repro here, 2020-07-06 in that thread.

In particular, at this 2020-07-15 message (just above the one you linked to about being unsure the server was doing the right thing), my recollection was that I didn't have the app open at all when I did that repro. That'd rule out being caused by the change we're talking about in this PR.

In that repro, I also saw the badge count successfully updating from 3 down to 1, which wouldn't be consistent with this bug being involved. It's just going from 1 down to 0 that didn't happen.

I think that last bug is the one I had in mind in the message you linked to.

gnprice · 2021-06-18T04:42:30Z

Oh, one other mystery, but this one is OK if it stays a mystery:

From reading the Apple docs following links from UNNotificationPresentationOptionBadge, I don't actually understand why the behavior in the status quo (before this PR) is the way it is.

Namely, when we get a notification while the app is in the foreground, no notification appears in the UI -- there's no banner at the top, and it's not in Notification Center when I swipe from the top of the screen to see that. I just tested that empirically:

on an iPad mini 4, running iOS 14.6;
with v27.164, the release I just made yesterday (and successfully uploaded today to alpha, and plan to send to beta right after I merge this PR);
by sending a PM from user A to user B, where user B is logged in on the device.
- User B has the "send mobile notifications even if offline" setting enabled, so the server should be sending the notification regardless of what the client is doing. But I haven't directly verified that it's doing so.
What I found is that I reliably get a notification -- both a banner, and in the list in Notification Center -- if the app is not foregrounded, and none if it is.

That behavior reflects what we're explicitly asking for with the change in this PR, passing just UNNotificationPresentationOptionBadge to the callback from userNotificationCenter:willPresentNotification:withCompletionHandler:.

But if you look at the docs for that method, they say:

If your delegate does not implement this method, the system behaves as if you had passed the UNNotificationPresentationOptionNone option to the completionHandler block. If you do not provide a delegate at all for the UNUserNotificationCenter object, the system uses the notification’s original options to alert the user.

The behavior we're seeing is consistent with UNNotificationPresentationOptionNone.

But in the status quo, I don't think it's the case that we have a delegate for UNUserNotificationCenter that doesn't implement this method. Rather, it's that we don't provide a delegate at all. Providing a delegate is what this diff is doing:

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

(… also, now that I've actually read some of the relevant API docs, I see that that comment is totally wrong. We're not defining that object -- the object already exists. The first line has us just getting a pointer to it, for us to use in the second line. The second line has us telling the center what object to use as its delegate object: namely, the AppDelegate object, the same one we use as the delegate for UIApplication.)

And with some grepping I'm not finding anywhere that our dependencies are providing such a delegate on our behalf, either. E.g. rg -C4 UNUserNotificationCenter node_modules/ finds references in a handful of files, and they're doing various things you might expect, but none of them appear to be setting the delegate property.

So it sure seems like the docs say we should be seeing this behavior:

If you do not provide a delegate at all for the UNUserNotificationCenter object [as we apparently don't], the system uses the notification’s original options to alert the user.

And what does it mean to "use" those "original options"? Well, there's this reference doc for composing an APNs payload.

We're providing an alert property -- that's what produces the banner and the Notification Center item, when the app isn't in the foreground. So it seems like the behavior we should expect to be seeing now is that it'd be producing the same banner and Notification Center item when the app is in the foreground, too.

We're also providing a badge property, of course, with a value of 0. So the same reading of the docs would also predict that we'd see the badge getting set to 0 (and more generally, to whatever the server said in the payload) regardless of whether we're in the foreground or background.

Anyway. EIBTI -- I'm glad to be making it explicit what we want in this situation, and no longer relying on that corner of the API's behavior. Regardless of whether the solution to the mystery is that it's not doing what the docs say it does, or that we are in fact "provid[ing] a delegate" in some nonobvious way.

…OS`. In an upcoming commit, we'll abandon `react-native-notifications` (which we'd only been using on iOS) in favor of PushNotificationIOS, provided by React Native (and then we'll take that same code from react-native-community, where it's maintained now). Once we've switched to PushNotificationIOS, none of the events we can listen to on iOS will match the Android events' names. That matching was pointed out in 4008643: """ And the names we use over the `DeviceEventEmitter` channel on Android are chosen to match the names the Wix library offers on its own event source... """ That meant that there were a few possible cases where we could call `this.listen` and pass a single name that wasn't conditioned on the platform. We took advantage of that possibility in one or two places, but we'll no longer be able to, unless we change the Android names to match these (keep reading for one reason not to do this). I'm happy to now have the `PushNotificationEventName` type from PushNotificationIOS (whether from RN or RNC) so we don't accidentally listen for events on iOS that won't be emitted. I'm optimistic that PushNotificationIOS may eventually have types that associate those names with the iOS-specific data shapes that are passed to their corresponding listeners. E.g., if I'm listening for the 'notification' event, my listener must only accept a PushNotificationIOS object. If such types are eventually provided, we'll want to use them -- but keep them away from Android codepaths. Having `this.listenIOS` and `this.listenAndroid` be separate, as well as keeping the events' names in different namespaces, will help with that. One gain from 8ee1793 is easily preserved: these helper functions still ensure that the appropriate functions are called to remove the listeners on `unlistenAll`.

This is based on the recommended setup for PushNotificationIOS from React Native [1], so we might have included it in the next commit, where we fully set that up. But as Greg points out [2], this code is purely using upstream iOS APIs, so it doesn't depend on whether we're using that RN module or something else, so it's fine to do here on its own. In a departure from those instructions, we decide to just pass the thing (`UNNotificationPresentationOptionBadge`) that updates the badge count when the app is foregrounded, and not ask for other things like showing the banner or playing a sound. (This new behavior is necessary for fixing zulip#4182, but I don't think it's sufficient: we'll still want to test that the server is sending the right badge counts to the client.) [1] https://reactnative.dev/docs/0.62/pushnotificationios. These instructions are actually missing a required change to the AppDelegate.h, which we take here, having seen it in the instructions for @react-native-community/push-notification-ios (at https://github.com/react-native-push-notification-ios/push-notification-ios#update-appdelegateh). [2] zulip#4163 (comment)

…ingly. As Greg says [1], "It actually seems hard to me to 100% confirm this from the implementation! [...] It might be simpler to appeal to what this method's job is: it's giving us the notification data, which was passed over APNs as JSON." So, do that. [1] zulip#4163 (comment)

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)

… core. In a recent commit, we asked RN's PushNotificationIOS to take responsibilities that were previously taken by wix/react-native-notifications, and removed the Wix library. Now, we account for the fact that PushNotificationsIOS from RN is deprecated [1] and asks us to use @react-native-community/push-notification-ios instead. So, do. These were my steps: 1. Use the setup instructions for `PushNotificationIOS` [1] from RN v0.62 to tear it down. 2. Follow the setup instructions for @react-native-community/push-notification-ios at the latest, v1.5.0 [2]. The native code closely matches what was there before, which makes sense. There are a few additions, and notes on old iOS APIs. We no longer support iOS 10, so we leave out some methods that target iOS <=10. 3. Follow the simple "Migrating..." instructions [3] that say you just have to change the imports; no call site changes are necessary. 4. Change a few comments that refer to details of the directory structure or implementation of the library. 5. Test thoroughly, as in the previous commit, and observe the same results. [1]: https://reactnative.dev/docs/0.62/pushnotificationios [2]: https://github.com/react-native-community/push-notification-ios/blob/v1.5.0/README.md [3]: https://github.com/react-native-community/push-notification-ios/blob/v1.5.0/README.md#migrating-from-the-core-react-native-module Fixes: zulip#4115

As Greg says [1], > Probably "interesting" wasn't a very clear way to write that > description; the distinction that I think it's intended to get at > is "unexpected", as in "buggy". [1] zulip#4163 (comment)

And throw an error if it gets one, since it definitely indicates a bug. See discussion at zulip#4163 (comment).

gnprice · 2021-06-18T04:58:25Z

I've also just sent a quick PR with a small followup refactor to the fromAPNs interface: #4810.

chrisbobbe · 2021-06-18T14:25:45Z

Thanks for the review and merge! 🙂

chrisbobbe added a-iOS a-notifications upstream: RN Issues related to an issue in React Native labels Jun 17, 2020

chrisbobbe requested a review from gnprice June 17, 2020 01:42

chrisbobbe commented Jun 17, 2020

View reviewed changes

gnprice reviewed Jun 17, 2020

View reviewed changes

chrisbobbe force-pushed the pr-remove-r-n-notifs branch from bbbf705 to 3d114e0 Compare July 13, 2020 22:00

chrisbobbe mentioned this pull request Aug 27, 2020

Show notifications for inactive (but logged-in) accounts while the app is foregrounded. #4241

Closed

chrisbobbe mentioned this pull request Sep 11, 2020

Pre-RN v0.61 -> v0.62 upgrade changes #4244

Merged

chrisbobbe force-pushed the pr-remove-r-n-notifs branch from 3d114e0 to 48e14db Compare September 16, 2020 00:25

chrisbobbe force-pushed the pr-remove-r-n-notifs branch from 48e14db to 1f86380 Compare September 16, 2020 00:28

chrisbobbe commented Sep 16, 2020

View reviewed changes

chrisbobbe force-pushed the pr-remove-r-n-notifs branch from 1f86380 to 2c077cb Compare November 10, 2020 01:28

gnprice reviewed Nov 10, 2020

View reviewed changes

chrisbobbe force-pushed the pr-remove-r-n-notifs branch from 2c077cb to 5bda5b5 Compare December 2, 2020 20:05

chrisbobbe force-pushed the pr-remove-r-n-notifs branch from 7915a3f to adbbab1 Compare May 10, 2021 17:40

gnprice mentioned this pull request May 26, 2021

[ios] Opening app from recents screen can reopen conversation from previous notification #4763

Open

chrisbobbe added the P1 high-priority label Jun 3, 2021

gnprice reviewed Jun 16, 2021

View reviewed changes

chrisbobbe added a commit to chrisbobbe/zulip-mobile that referenced this pull request Jun 17, 2021

notification: Have fromAPNs accept a nullish value.

6949325

And throw an error if it gets one, since it definitely indicates a bug. See discussion at zulip#4163 (comment).

chrisbobbe force-pushed the pr-remove-r-n-notifs branch from adbbab1 to 6949325 Compare June 17, 2021 02:20

chrisbobbe added 8 commits June 17, 2021 21:42

notifications [nfc]: Name an iOS event handler more clearly.

b6caf19

notification: Make a bit of fromAPNsImpl's jsdoc more precise.

fea51bb

As Greg says [1], > Probably "interesting" wasn't a very clear way to write that > description; the distinction that I think it's intended to get at > is "unexpected", as in "buggy". [1] zulip#4163 (comment)

notification: Have fromAPNs accept a nullish value.

908f765

And throw an error if it gets one, since it definitely indicates a bug. See discussion at zulip#4163 (comment).

gnprice force-pushed the pr-remove-r-n-notifs branch from 6949325 to 908f765 Compare June 18, 2021 04:43

gnprice merged commit 908f765 into zulip:master Jun 18, 2021

gnprice mentioned this pull request Jun 18, 2021

notif ios [nfc]: Factor out fromPushNotificationIOS. #4810

Merged

chrisbobbe mentioned this pull request Jun 22, 2021

package.json [nfc]: Re-alphabetize dependencies. #4816

Merged

chrisbobbe deleted the pr-remove-r-n-notifs branch November 5, 2021 00:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start using @react-native-community/push-notification-ios #4163

Start using @react-native-community/push-notification-ios #4163

chrisbobbe commented Jun 17, 2020 •

edited

Loading

chrisbobbe Jun 17, 2020 •

edited

Loading

chrisbobbe Jun 17, 2020

gnprice Nov 10, 2020

gnprice Nov 10, 2020

gnprice left a comment

gnprice Jun 17, 2020

chrisbobbe Jun 30, 2020

chrisbobbe Sep 16, 2020 •

edited

Loading

chrisbobbe commented Jun 30, 2020 •

edited

Loading

chrisbobbe commented Jul 2, 2020

chrisbobbe commented Sep 16, 2020 •

edited

Loading

chrisbobbe Sep 16, 2020

chrisbobbe commented Nov 10, 2020 •

edited

Loading

gnprice left a comment

gnprice Nov 10, 2020

gnprice Nov 10, 2020

gnprice Nov 10, 2020

chrisbobbe commented Nov 24, 2020

chrisbobbe commented Dec 2, 2020

chrisbobbe commented Dec 2, 2020

chrisbobbe commented May 10, 2021

gnprice left a comment

gnprice Jun 16, 2021

chrisbobbe Jun 16, 2021 •

edited

Loading

gnprice Jun 18, 2021

chrisbobbe commented Jun 17, 2021 •

edited

Loading

chrisbobbe commented Jun 17, 2021

gnprice commented Jun 18, 2021

gnprice commented Jun 18, 2021

gnprice commented Jun 18, 2021

chrisbobbe commented Jun 18, 2021

Start using @react-native-community/push-notification-ios #4163

Start using @react-native-community/push-notification-ios #4163

Conversation

chrisbobbe commented Jun 17, 2020 • edited Loading

chrisbobbe Jun 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Jun 30, 2020 • edited Loading

chrisbobbe commented Jul 2, 2020

chrisbobbe commented Sep 16, 2020 • edited Loading

Choose a reason for hiding this comment

chrisbobbe commented Nov 10, 2020 • edited Loading

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Nov 24, 2020

chrisbobbe commented Dec 2, 2020

chrisbobbe commented Dec 2, 2020

chrisbobbe commented May 10, 2021

gnprice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jun 17, 2021 • edited Loading

chrisbobbe commented Jun 17, 2021

gnprice commented Jun 18, 2021

gnprice commented Jun 18, 2021

gnprice commented Jun 18, 2021

chrisbobbe commented Jun 18, 2021

chrisbobbe commented Jun 17, 2020 •

edited

Loading

chrisbobbe Jun 17, 2020 •

edited

Loading

chrisbobbe Sep 16, 2020 •

edited

Loading

chrisbobbe commented Jun 30, 2020 •

edited

Loading

chrisbobbe commented Sep 16, 2020 •

edited

Loading

chrisbobbe commented Nov 10, 2020 •

edited

Loading

chrisbobbe Jun 16, 2021 •

edited

Loading

chrisbobbe commented Jun 17, 2021 •

edited

Loading