Fix ReactNotificationService for notifications between app and modules #6877
Conversation
| }; | ||
|
|
||
| // The ReactNotificationService manages notification subscriptions and sending messages. | ||
| // A notification service may have a parent notification service most for the automatic |
There was a problem hiding this comment.
can you clarify what this means? most what?
| // The ReactNotificationService manages notification subscriptions and sending messages. | ||
| // A notification service may have a parent notification service most for the automatic | ||
| // unsubscription when the notification service is deleted. | ||
| // All notifications are always sent from the root notification service that doe snot have parent service. |
| // method called on the "child" service just calls "parent" service's SendNotification method. | ||
| // - Subscriptions are added to services on all levels: a subscription added to a "child" service is also | ||
| // added to the "parent" service. | ||
| // - When "child" subscription is removed, it also removes the corresponding subscription in the |
There was a problem hiding this comment.
Do we have a risk of folks adding the same subscription to two nodes? i.e add it to root and then create a child to which it is added?
Of with the use of Lambda's to register the notifications that risk does not really exist... Or does the c++ parameterless lambda do some shared instance management?
There was a problem hiding this comment.
Each subscription is an object and it is unique. The Unsubscribe method is only available on the IReactNotificationSubscription object, and not on the IReactNotificationService. I.e. every time we call Subscribe method we create a unique subscription even if some of them have the same notification Id and the callback/lambda.
There was a problem hiding this comment.
| TestEventService::Initialize(); | ||
|
|
||
| auto reactNativeHost = | ||
| TestReactNativeHostHolder(L"ReactNotificationServiceTests", [](ReactNativeHost const &host) noexcept { |
There was a problem hiding this comment.
I realize these are integration tests, should we have some unittests for this as well, especially to test things like:
- subscribe, unsubscribe, subscribe
- nested notification services etc
- subscribing twice on the same node
- subscribing twice on sibblings and then unsubscribe.
- Error case for removing subscription twice?
There was a problem hiding this comment.
Good point! Some of these tests are already there: see other tests in the same test file. I want to address the current issue as soon as possible, and I will address the additional test scenarios in another PR. I have created the issue #6881 for these items to be on my radar.
There was a problem hiding this comment.
vmoroz
added
the
AutoMerge
|
Hello @vmoroz! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 10 hours, a condition that will be fulfilled in about 8 hours 11 minutes. No worries though, I will be back when the time is right! 😉 p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit * Fix unused parameter warning in AbiCallInvoker (#6860) * Move CallInvoker.h to TurboModule filter folder (#6861) * Simplify Microsoft.ReactNative.IntegrationTests (#6868) * Update clang-format version to 1.5.0 (#6870) * Add JSValue constructor for std::optional<T> (#6873) * Updated doc comments in IReactNotificationService.idl (#6875) * Fix ReactNotificationService for notifications between app and modules (#6877)
3 participants
This PR addressed the issue described in #6836 and #6840.
The
ReactNotificationServiceis used to send notifications between native modules, and between native modules and the app.To provide automatic unsubscription on instance unload for all subscriptions created in native modules, we have implemented parent-child relationship between the
ReactNotificationService("parent") created inReactNativeHost::InstanceSettingsand in theReactContext("child"). The issue was that while notifications from the "child" service is propagated to the "parent", there was no mechanism to propagate them from the "parent" to "children".In this PR we fix the issue. While we keep the same parent-child relationship, we change the way the notifications are sent and how we subscribe to them:
method called on the "child" service just calls "parent" service's SendNotification method.
added to the "parent" service.
"parent" service.
Other related changes in this PR:
NotificationBetweenAppAndModuletest to verify the new behavior.IReactNotificationSubscription::NotificationServiceas a convenience property to be used in the notification handler.std::unordered_mapfromstd::mapto keep subscriptions. It should have a better perf.JSValueconstructors inTestEventServiceandTestEvent.Microsoft Reviewers: Open in CodeFlow