[expo-notifications] Add useLastNotificationResponse hook by sjchmiela · Pull Request #10883 · expo/expo

sjchmiela · 2020-10-31T16:10:52Z

Why

Idea inspired by @cruzach sounded! #10811 (review)

How

Since there will be two hooks now that depend on the notification response listener being registered in the global scope I moved it to a SharedNotificationResponseListener.fx.ts file
Changed useInitialNotificationResponse implementation to rely on it
Added the ability to listen for incoming responses to the shared listener
Implemented useLastNotificationResponse which is like a notification response swiss knife:
- if registered early enough judging by its return values states you can deduce whether there was this special "initial notification response" or not
- always returns the latest notification response received (or something falsy, depending on whether you care about the "initiality" or "initialness" of the response you can ignore the difference between undefined or null or not)

Frankly speaking I'm not 100% sure how big of a deal is leaving a dangling listener active, but I can't think of any reason why it would be a problem.

Test Plan

I have verified this hook returns latest notification response if the hook is mounted as (early in the app as possible and after some time from starting the app) ✖️ (the app is started in response to notification tap, notification tap while the app is running)

github-actions · 2020-11-10T16:50:47Z

Native Component List for this branch is ready

cruzach

I tested this (using the code from this snack), and I'm consistently getting the initial (i.e. app-launching) notification response!! 🎉 only thing I want to bring up is that I'm seeing some different behavior on iOS vs Android when using both useLastNotificationResponse and addNotificationResponseReceivedListener at the same time. I'm not sure about a use case that would use both, but just because the behavior was different on each platform I thought it might be worth bringing up here:

iOS

only useLastNotificationResponse triggered if app is dead & opened with notification
only addNotificationResponseReceivedListener called if app is foregrounded or backgrounded & you tap on a notification

Android

only useLastNotificationResponse triggered if app is dead & opened with notification
both hook AND listener called if app is foregrounded or backgrounded

Sidenote- looks like RN is also sending a warning when you open the app via a notification (this happened to me in development on iOS):

Sending notification when app is killed, on open: Sending onDidReceiveNotificationResponse with no listeners registered.

Should/could we silence that?

docs/pages/versions/unversioned/sdk/notifications.md

cruzach

sorry- forgot to add this to the last review

cruzach · 2020-11-12T16:51:45Z

...id/src/main/java/expo/modules/notifications/notifications/emitting/NotificationsEmitter.java

    mNotificationManager.removeListener(this);
  }

+  public void getLastNotificationResponseAsync(Promise promise) {


should this be an @ExpoMethod?

Of coourse! Thank you for catching that, I was so worked up on iOS not working I forgot to test Android!

The listener will be reused by two hooks — useLastNotificationResponse and useInitialNotificationResponse

…SharedNotificationResponseListener

…ing from undefined state more quickly BTW also making EXNotificationsEmitter always listen to events from EXNotificationCenterDelegate

…hich could remove `useLastNotificationResponse` listeners

Co-authored-by: Charlie Cruzan <35579283+cruzach@users.noreply.github.com>

sjchmiela · 2020-11-13T19:59:23Z

I have now:

tested it works on Android as expected
removed the RN warning of "Sending onDidReceiveNotificationResponse with no listeners registered."
removed removeAllNotificationListeners() method — I suspect the dichotomy between Android and iOS with useLastNotificationResponse behavior was resulting from useLastNotificationResponse's listener being erroneously removed by the removeAll….

cruzach

LGTM 🙌

Last question- from the looks of it, I don't think any scoped Android code needs to be updated, but EXScopedNotificationsEmitter.m does need to be updated so that this fix lands in the managed workflow for SDK 40? I suppose it might make more sense for that to land in a separate PR though (up to you)

sjchmiela · 2020-11-13T21:21:47Z

Thank you for catching that! 👁️ 🕵️ 3953aa4 should take care of it by changing the way we override serialization mechanism and thanks to that allowing us to call super in callback methods.

cruzach

perfect! thanks for adding that in so quickly 🏎️

…ion response

SimenB · 2020-12-02T12:01:55Z

Sorry to ask here, but is this hook the recommended way of handling notification presses? The guide does not use it, but it's a class component: https://docs.expo.io/push-notifications/receiving-notifications/. It also seems to use the same API as this hook does under the hood.

Currently trying to migrate to expo-notifications after pulling out in both SDK 38 and 39, and having issues with presses not triggering our handlers correctly when app is in background. Legacy notifications work correctly

cruzach · 2020-12-03T16:28:28Z

@SimenB Yes, but this hook landed as part of SDK 40 (which is currently in beta)

You can see documentation for it here, but since it's still in beta i think we're going to keep the example you linked to the same for now 👍

alaminopu · 2020-12-04T02:17:52Z

packages/expo-notifications/README.md

-      initialNotificationResponse.notification.data.url &&
-      initialNotificationResponse.actionIdentifier === Notifications.DEFAULT_ACTION_IDENTIFIER
+      lastNotificationResponse &&
+      lastNotificationResponse.notification.data.url &&


@sjchmiela I was wondering if this is the correct way to access notification.data, since lastNotificationResponse contain the response from addNotificationResponseReceivedListener. Shouldn't it be notification.request.content.data ?

SimenB · 2020-12-04T09:22:17Z

@SimenB Yes, but this hook landed as part of SDK 40 (which is currently in beta)

Yep, I'm testing with the beta 👍 Thanks for the response, 🤞 this works out for us

…nly once JS subscribes to it (#11382) # Why Fixes a bug introduced in #10883 which causes initial notification responses to get lost in standalone iOS apps. Along with #11378 I believe it fixes #11343 and may also finally fix #9866. # How Using native breakpoints I've noticed `EXNotificationCenterDelegate`'s `addDelegate:` is called more times than I would expect it to. It turned out before `EXScopedNotificationsEmitter` was subscribing to `EXNotificationCenterDelegate` (it would then receive pending notification response) an unscoped `EXNotificationsEmitter` was subscribing and consuming pending responses. Since unscoped emitter wasn't being used and exposed to the app later, it consumed the response into the void. Most probably unimodules have a bug where instances of overridden modules are still kept in memory (since `setModuleRegistry:` is called on them). Fixing this bug is out of the scope of this pull request. These changes are based on how it was implemented before dfda625. I can't see any reason why `EXNotificationsEmitter` would need to register for notifications as soon as module registry is ready, so let's roll back to what we know worked well. # Test Plan I have verified in an ExpoKit workspace that when the app is opened from a notification, the notification response is delivered to `useLastNotificationResponse`: https://www.loom.com/share/586a906c538f47f0940b2cc83582fb29

…nly once JS subscribes to it (#11382) # Why Fixes a bug introduced in expo/expo#10883 which causes initial notification responses to get lost in standalone iOS apps. Along with #11378 I believe it fixes #11343 and may also finally fix #9866. # How Using native breakpoints I've noticed `EXNotificationCenterDelegate`'s `addDelegate:` is called more times than I would expect it to. It turned out before `EXScopedNotificationsEmitter` was subscribing to `EXNotificationCenterDelegate` (it would then receive pending notification response) an unscoped `EXNotificationsEmitter` was subscribing and consuming pending responses. Since unscoped emitter wasn't being used and exposed to the app later, it consumed the response into the void. Most probably unimodules have a bug where instances of overridden modules are still kept in memory (since `setModuleRegistry:` is called on them). Fixing this bug is out of the scope of this pull request. These changes are based on how it was implemented before expo/expo@dfda625. I can't see any reason why `EXNotificationsEmitter` would need to register for notifications as soon as module registry is ready, so let's roll back to what we know worked well. # Test Plan I have verified in an ExpoKit workspace that when the app is opened from a notification, the notification response is delivered to `useLastNotificationResponse`: https://www.loom.com/share/586a906c538f47f0940b2cc83582fb29

sjchmiela force-pushed the @sjchmiela/expo-notifications-useinitialnotificationresponse branch from 0393f63 to b1e275f Compare November 9, 2020 09:45

Base automatically changed from @sjchmiela/expo-notifications-useinitialnotificationresponse to master November 10, 2020 16:16

sjchmiela force-pushed the @sjchmiela/expo-notifications-uselastnotificationresponse branch from e2fd3f9 to 5191e72 Compare November 10, 2020 16:21

sjchmiela force-pushed the @sjchmiela/expo-notifications-uselastnotificationresponse branch 2 times, most recently from 5fe5ddd to e909084 Compare November 10, 2020 19:59

sjchmiela marked this pull request as ready for review November 12, 2020 09:18

sjchmiela requested a review from cruzach as a code owner November 12, 2020 09:18

cruzach reviewed Nov 12, 2020

View reviewed changes

sjchmiela added 8 commits November 13, 2020 20:27

[expo-notifications] Add SharedNotificationResponseListener

2c78333

The listener will be reused by two hooks — useLastNotificationResponse and useInitialNotificationResponse

[expo-notifications] Add the ability to register for new response to …

bd7f750

…SharedNotificationResponseListener

[expo-notifications] Add useLastNotificationResponse hook

f30d53a

[expo-notifications] Add CHANGELOG entry

447cfbb

[docs][expo-notifications] Add documentation

1a1e570

[expo-notifications] Remove useInitialNotificationResponse hook

00a51a0

[expo-notifications] Fetch latest response from native module, resolv…

dfda625

…ing from undefined state more quickly BTW also making EXNotificationsEmitter always listen to events from EXNotificationCenterDelegate

[expo-notifications] Remove removeAllNotificationListeners method w…

fe0bdb8

…hich could remove `useLastNotificationResponse` listeners

sjchmiela force-pushed the @sjchmiela/expo-notifications-uselastnotificationresponse branch from c02e294 to fe0bdb8 Compare November 13, 2020 19:27

Apply suggestions from code review

3dc5eb4

Co-authored-by: Charlie Cruzan <35579283+cruzach@users.noreply.github.com>

sjchmiela force-pushed the @sjchmiela/expo-notifications-uselastnotificationresponse branch from 3665d66 to 3dc5eb4 Compare November 13, 2020 19:30

[expo-notifications] Silence RN warning regression introduced in dfda625

a45edb7

sjchmiela requested a review from cruzach November 13, 2020 20:48

cruzach approved these changes Nov 13, 2020

View reviewed changes

sjchmiela requested a review from cruzach November 13, 2020 21:21

cruzach approved these changes Nov 13, 2020

View reviewed changes

[ios] Fix EXScopedNotificationsEmitter not registering last notificat…

9f85299

…ion response

sjchmiela force-pushed the @sjchmiela/expo-notifications-uselastnotificationresponse branch from 3953aa4 to 9f85299 Compare November 14, 2020 12:10

sjchmiela merged commit 1037e82 into master Nov 14, 2020

sjchmiela deleted the @sjchmiela/expo-notifications-uselastnotificationresponse branch November 14, 2020 12:11

alaminopu reviewed Dec 4, 2020

View reviewed changes

sjchmiela mentioned this pull request Dec 16, 2020

[expo-notifications] Register emitter as center delegate's delegate only once JS subscribes to it #11382

Merged

brentvatne mentioned this pull request Jul 27, 2021

expo-notifications for SDK 40 does not have removeAllNotificationListeners [docs] #13791

Closed

Conversation

sjchmiela commented Oct 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

How

Test Plan

Uh oh!

github-actions bot commented Nov 10, 2020

Uh oh!

cruzach left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cruzach left a comment

Choose a reason for hiding this comment

Uh oh!

cruzach Nov 12, 2020

Choose a reason for hiding this comment

Uh oh!

sjchmiela Nov 13, 2020

Choose a reason for hiding this comment

Uh oh!

sjchmiela commented Nov 13, 2020

Uh oh!

cruzach left a comment

Choose a reason for hiding this comment

Uh oh!

sjchmiela commented Nov 13, 2020

Uh oh!

cruzach left a comment

Choose a reason for hiding this comment

Uh oh!

SimenB commented Dec 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cruzach commented Dec 3, 2020

Uh oh!

alaminopu Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

SimenB commented Dec 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sjchmiela commented Oct 31, 2020 •

edited

Loading

SimenB commented Dec 2, 2020 •

edited

Loading