-
Notifications
You must be signed in to change notification settings - Fork 1.2k
ReactNotificationService to allow communication between native modules #4953
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| explicit ReactNotificationService(IReactNotificationService const parentNotificationService) noexcept; | ||
| ~ReactNotificationService() noexcept; | ||
|
|
||
| IReactNotificationSubscription Subscribe( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subscribe [](start = 33, length = 9)
Just curious, we have CallbackStore for events in devmain, would it make sense to use something like that here? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This implementation uses a lot of ideas from the CallbackStore. But, it has some more advanced features such as the IReactSubscription object. It is much more powerful when retuning just a 'cookie'
In reply to: 427618875 [](ancestors = 427618875)
|
|
||
| IReactPropertyName NotificationName() const noexcept; | ||
| IReactDispatcher Dispatcher() const noexcept; | ||
| bool IsSubscribed() const noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IsSubscribed [](start = 7, length = 12)
Why is this needed? Wouldn't it create race condition if someone calls if (IsSubscribed()) call()? (but someone unsubscribes in-between) #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another way of putting it, what information does return value from this function give? If after returning false, there may still be callbacks en-route.
In reply to: 427621090 [](ancestors = 427621090)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is useful inside of the notification handler. We check this property before we call the notification handler, but like you said the notification could be in-route. The only thread-safe to ensure that no code executed after unsubscription is to do another check inside of handler. It should be done only for special cases involved multi-threading, but at least we allow it.
In normal cases developers do not need to check this property.
In reply to: 427632411 [](ancestors = 427632411,427621090)
| winrt::Microsoft::ReactNative::IReactPropertyBag const &properties, | ||
| winrt::Microsoft::ReactNative::IReactNotificationService const ¬ifications) noexcept; | ||
|
|
||
| void Destroy() noexcept; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Destroy [](start = 7, length = 7)
Can you add quick comment on why should users need this as opposed to just having destructor taking care of this? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| struct ReactDispatcher { | ||
| ReactDispatcher(std::nullptr_t = nullptr) noexcept {} | ||
|
|
||
| explicit ReactDispatcher(IReactDispatcher const &handle) noexcept : m_handle{handle} {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
& [](start = 49, length = 2)
(nit) Since we are doing assignment anyways, wouldn't && be better here? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is the C++/WinRT thing. It always gives us const &.
In reply to: 427624661 [](ancestors = 427624661)
| } | ||
|
|
||
| void Post(ReactDispatcherCallback const &callback) const noexcept { | ||
| if (m_handle) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
m_handle [](start = 8, length = 8)
(nit) can you add comment of why we would ever want this to be null? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class supports move semantic. It can be null if its content is moved out, or nullptr is given at construction.
In reply to: 427625320 [](ancestors = 427625320)
| // Consider calling the Unsubscribe method and the handler in the same IReactDispatcher | ||
| // to ensure that no handler is invoked after the Unsubscribe method call. | ||
| void Unsubscribe() const noexcept { | ||
| if (m_handle) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (m_handle) { [](start = 4, length = 15)
Similar to previous comment, why would we ever allow null handle? Wouldn't it be easier if m_handle is always an object? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the other answer and the constructor that accepts nullptr.
In reply to: 427626890 [](ancestors = 427626890)
NikoAri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
![]()
|
@vmoroz I know we talked offline about this a bit, but I don't think this should be targeting 0.62. It's pretty much out the door, and we won't be able to iterate on this like we've done for other recent APIs. |
|
is it by design that the notification service is per react context? |
| } | ||
|
|
||
| explicit operator bool() const noexcept { | ||
| return static_cast<bool>(m_handle); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider making this explicit by comparing against nullptr instead #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change it to code that C++/WinRT uses:
m_handle ? true : false;
Comparing with nullptr is more expensive as it involves calling a constructor with nullptr parameter and then comparing two objects.
In reply to: 427648694 [](ancestors = 427648694)
| // True if two types with Handle() have different handles. | ||
| template <class T, std::enable_if_t<HasHandleV<T>, int> = 0> | ||
| inline bool operator!=(T const &left, T const &right) noexcept { | ||
| return left.Handle() != right.Handle(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider not using the != operator, and instead use !(left.Handle() == right.Handle()) #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| namespace winrt::Microsoft::ReactNative { | ||
|
|
||
| // Encapsulates the IReactPropertyName and the notification data type | ||
| template <class T> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider simplifying this by just making this an alias instead of inheriting from it #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see how it can be done. The alias will not allow me to access the template type. Inheritance here is cheap: we inherit from a non-template class.
In reply to: 427649718 [](ancestors = 427649718)
| }; | ||
|
|
||
| struct ReactNotificationArgsBase { | ||
| ReactNotificationArgsBase(std::nullptr_t = nullptr) noexcept {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mark the ctor as protected so we don't create this from outside the subclasses? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // ReactPropertyName encapsulates the IReactPropertyName. | ||
| // It represents an atomic property name object that defines a LocalName | ||
| // within the referenced Namespace. | ||
| struct ReactPropertyName { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the name Name would seem to hint that this is the leaf name i.e. the local name (whereas Id can be global). Curious to understand the reasoning behind the rename? #Resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I need to reuse it for the ReactNotificationId. Plus, it became a little bit more efficient since we have less method implementations that depend on T.
In reply to: 427651462 [](ancestors = 427651462)
|
In the current design we have two levels of Notifications: the top level is in the ReactInstanceSettings.Notifications. They have the same lifespan as the ReactInstanceSettings which is usually given to ReactNativeHost. Then we create a new ReactContext.Notifications that use the ReactInstanceSettings.Notifications as parent. The SendNotification will use them both, while subscribe goes only to ReactContext.Notifications. All ReactContext.Notifications are automatically unsubscribed when we unload the React instance and destroy the ReactContext. This design allows modules and views to talk to each other, and the ReactInstanceSettings.Notifications opens communication with the hosting application. Re: Should components from different react contexts be able to communicate?
In reply to: 631132307 [](ancestors = 631132307) |
|
I believe this API is quite important for the 0.62 release as we plan to implement a lot of new native modules. In reply to: 631128574 [](ancestors = 631128574) |
Because IReactPropertyNamespace is an atomized object that groups IReactPropertyName instances. In reply to: 631103652 [](ancestors = 631103652) Refers to: vnext/Microsoft.ReactNative/IReactPropertyBag.idl:28 in 696d8c6. [](commit_id = 696d8c6, deletion_comment = False) |
|
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 3 hours 38 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 (
|
|
We're not backporting additional payload to 0.62 without triage at this time. |
|
Discussed this offline. @vmoroz it would be good to discuss this one at triage. I'll send you the invite. |
|
Thanks @vmoroz for adding this. About back porting, the reanimated module I'm working on will need this to listen on some UIManager events (just providing some more data points for you guys to consider when having the triage discussion 😃). |
microsoft#4953) * ReactNotificationService to allow communications between native modules * Change files * Addressed code review feedback. * Fixed build issues
|
Based on earlier discussion during triage it sounded like we wanted to wait until we had the UIManager part of this, and a better understanding of timing of dependents before considering backporting, Updating labels on this PR. @vmoroz please feel free to re-add the labels if you believe this is incorrect. |
* Implemented support for native module std::weak_ptr (#4980) (cherry picked from commit c23d3de) # Conflicts: # packages/microsoft-reactnative-sampleapps/windows/SampleLibraryCPP/SampleModuleCPP.h * Fix ReactInstance error state to avoid crashes (#4986) * Fix ReactInstance error state to avoid crashes * Change files * Clean m_redboxContent after RedBox closing (cherry picked from commit 05779f2) * ReactNotificationService to allow communication between native modules (#4953) Only picking up changes required for ReactDispatcher. (cherry picked from commit baba475) * Add UIDispatcher property to ReactInstanceSettings and IReactContext (#5007) * Add UIDispatcher property to ReactInstanceSettings and IReactContext * Change files * Fix ReactWindows-Desktop.sln compilation * Address PR feedback for the GetCurrentUIThreadQueue * Remove UIDispatcher setting from Playground project (cherry picked from commit 7236d61) # Conflicts: # packages/playground/windows/playground-win32/Playground-Win32.cpp # vnext/Desktop/React.Windows.Desktop.vcxproj.filters # vnext/Microsoft.ReactNative.Cxx.UnitTests/ReactContextTest.cpp # vnext/Microsoft.ReactNative.Cxx.UnitTests/ReactModuleBuilderMock.h # vnext/Microsoft.ReactNative.Cxx/ReactContext.h # vnext/Microsoft.ReactNative.Managed.UnitTests/ReactModuleBuilderMock.cs # vnext/Microsoft.ReactNative/IReactContext.cpp # vnext/Microsoft.ReactNative/IReactContext.h # vnext/Microsoft.ReactNative/IReactContext.idl # vnext/Microsoft.ReactNative/ReactInstanceSettings.h # vnext/Microsoft.ReactNative/ReactInstanceSettings.idl * Fix PlaygroundWin32 compilation
|
We decided not to backport this one. If this ends up being needed by a customer on RN 62, we will retriage. |
|
This PR introduces a new ReactNotificationService that allows native modules to communicate between each other, communicate with view managers, and with the application.
One examples of the usage is when a native module wants to listen to UIManager events. Instead of receiving UIManager as a parameter, the native module can just subscribe to the UIManager events in its
Initializemethod using theReactContext::Notifications()property.The
IReactNotificationServicehas two methods:Subscribeto subscribe to notification. It receivesIReactPropertyNameas a name of the notification (we reuse the PropertyBag's atomized names in this context), an optionalIReactDispatcherwhere the notification must be handled, and the handler to handle notification. TheSubscribemethod returnsIReactNotificationSubscriptionthat has the subscription name, dispatcher, andUnsubscribemethod to unsubscribe from the notification. ItsIsSubscribedproperty can be used to check if subscription is still active.SendNotificationto send a notification. It gets the notification name, sender object, and notification data object. The sender and data objects are optional and can be null.The notification handler receives sender object (
IInspectablein C++) andIReactNotificationArgsthat hasSubscriptionandDataproperties. TheSubscriptionproperty has the same value as the result of theSubscribemethod call and can be used to get the subscription properties and to be able to unsubscribe inside of the handler. TheDataproperty provides data sent by theSendNotificatondata.On top of the ABI-safe API, we offer wrappers for all new types:
ReactNotificationService,ReactNotificationSubscription,ReactNotificationArgs,ReactNotificationIdandReactDispatcher.The
ReactNotificationIdis a template that allows to specify an ABI-safe type to be used as notification data. The type can bevoid. For non-ABI safe value in local context we can useReactNonAbiValue<T>type.The code below is taken from the sample apps to show usage of the new ReactNotificationService:
The C# usage looks verbose. We will add helper classes in future PR.
The next examples are taken from our unit tests.
This is how we can unsubscribe from the notifications:
ReactNotificationService rns{ReactNotificationServiceHelper::CreateNotificationService()}; ReactNotificationId<void> fooNotification{L"Foo"}; bool isCalled{false}; auto subscription = rns.Subscribe( fooNotification, [&](IInspectable const & /*sender*/, ReactNotificationArgs<void> const & /*args*/) noexcept { isCalled = true; }); rns.SendNotification(fooNotification); TestCheck(isCalled); subscription.Unsubscribe(); TestCheck(!subscription.IsSubscribed()); isCalled = false; rns.SendNotification(fooNotification); TestCheck(!isCalled);We can use the
winrt::auto_revokevalue to getReactNotificationSubscriptionRevokerinstead ofReactNotificationSubscription:ReactNotificationService rns{ReactNotificationServiceHelper::CreateNotificationService()}; ReactNotificationId<void> fooNotification{L"Foo"}; bool isCalled{false}; auto revoker = rns.Subscribe(winrt::auto_revoke, fooNotification, [&](IInspectable const & /*sender*/, ReactNotificationArgs<void> const & /*args*/) noexcept { isCalled = true; }); rns.SendNotification(fooNotification); TestCheck(isCalled); revoker = nullptr; // destruction of revoker causes unsubscription. TestCheck(!subscription.IsSubscribed()); isCalled = false; rns.SendNotification(fooNotification); TestCheck(!isCalled);We can unsubscribe from inside of the the handler:
ReactNotificationService rns{ReactNotificationServiceHelper::CreateNotificationService()}; ReactNotificationId<void> fooNotification{L"Foo"}; bool isCalled{false}; auto subscription = rns.Subscribe( fooNotification, [&](IInspectable const & /*sender*/, ReactNotificationArgs<void> const &args) noexcept { isCalled = true; args.Subscription().Unsubscribe(); }); rns.SendNotification(fooNotification); TestCheck(isCalled); isCalled = false; rns.SendNotification(fooNotification); TestCheck(!isCalled);We can provide a
ReactDispatcherwhere to handle notification:ReactNotificationService rns{ReactNotificationServiceHelper::CreateNotificationService()}; ReactNotificationId<ReactNonAbiValue<std::string>> fooNotification{L"Foo"}; Mso::ManualResetEvent finishedEvent; ReactDispatcher dispatcher{ReactDispatcher::CreateSerialDispatcher()}; bool isCalled{false}; rns.Subscribe(fooNotification, dispatcher, [&](IInspectable const & /*sender*/, ReactNotificationArgs<ReactNonAbiValue<std::string>> const &args) noexcept { TestCheckEqual("Hello", *args.Data()); TestCheckEqual(dispatcher, args.Subscription().Dispatcher()); TestCheck(dispatcher.HasThreadAccess()); isCalled = true; finishedEvent.Set(); }); rns.SendNotification(fooNotification, "Hello"); finishedEvent.Wait(); TestCheck(isCalled);Microsoft Reviewers: Open in CodeFlow