[expo-notifications] Register emitter as center delegate's delegate only once JS subscribes to it #11382
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…egate only as soon as it's being observed Otherwise we hit a bug where in standalone apps two notification emitters are alive and get added as delegates to notification center delegate — the first, unscoped one consumes pending notification responses and once the second, scoped, proper one is being registered all the pending responses are lost.
…e being observed to sendEventWithName:
|
Native Component List for this branch is ready |
cruzach
approved these changes
Dec 16, 2020
There was a problem hiding this comment.
thanks for the video 🙏 this lgtm!
I don't believe there would be any difference if the notification comes in while the app is closed, but it might be worth double-checking? I.e. you schedule a notification for 5 seconds from now, kill the app, notification arrives, tap on it to launch app, do we still get the response? Again, I don't think it will make a difference but just me being extra-cautious
Approved but we should backport to sdk 40 versioned code as well
packages/expo-notifications/ios/EXNotifications/Notifications/Emitter/EXNotificationsEmitter.m
Show resolved
Hide resolved
sjchmiela
deleted the
@sjchmiela/fix-expo-notifications-initial-notification-expo
branch
sjchmiela
added a commit
that referenced
this pull request
Dec 16, 2020
…livered unless they're delivered to expo-notifications (#11378) # Why It's a nice counterpart to #11382 which ensures #11343 and #9866 do not happen again. # How Follow-up to #9478. There are two places where the notification response can be "consumed" — when it is received (covered by #9478) and when it is delivered to a new delegate (covered by this PR). # Test Plan I have verified in #11382 that workspace with this and #11382 changes delivers initial notification response to the app.
sjchmiela
added a commit
that referenced
this pull request
Dec 16, 2020
…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
sjchmiela
added a commit
that referenced
this pull request
Dec 16, 2020
…livered unless they're delivered to expo-notifications (#11378) # Why It's a nice counterpart to #11382 which ensures #11343 and #9866 do not happen again. # How Follow-up to #9478. There are two places where the notification response can be "consumed" — when it is received (covered by #9478) and when it is delivered to a new delegate (covered by this PR). # Test Plan I have verified in #11382 that workspace with this and #11382 changes delivers initial notification response to the app.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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'saddDelegate:is called more times than I would expect it to.It turned out before
EXScopedNotificationsEmitterwas subscribing toEXNotificationCenterDelegate(it would then receive pending notification response) an unscopedEXNotificationsEmitterwas 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
EXNotificationsEmitterwould 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