Updated doc comments in IReactNotificationService.idl #6875
Conversation
This file contains hidden or 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
asklar
reviewed
Jan 15, 2021
Comment on lines
+21
to
+23
| "The @IReactDispatcher provided when the notification subscription created.\n" | ||
| "All notifications will be handled using this dispatcher.\n" | ||
| "In case if it is null, the events are handled synchronously.") |
Member
There was a problem hiding this comment.
Suggested change
| "The @IReactDispatcher provided when the notification subscription created.\n" | |
| "All notifications will be handled using this dispatcher.\n" | |
| "In case if it is null, the events are handled synchronously.") | |
| "The @IReactDispatcher that was provided when the notification subscription was created.\n" | |
| "All notifications will be handled using this dispatcher.\n" | |
| "If the dispatcher is null, events will be handled synchronously.") |
| DOC_STRING("True if the subscription is still active. This property is checked before notification handler is invoked.") | ||
| DOC_STRING( | ||
| "True if the subscription is still active.\n" | ||
| "This property is checked before notification handler is invoked.") |
Member
There was a problem hiding this comment.
Suggested change
| "This property is checked before notification handler is invoked.") | |
| "This property is checked before the notification handler is invoked.") |
| { | ||
| // Create new instance of IReactNotificationService | ||
| DOC_STRING("Create new instance of IReactNotificationService") | ||
| DOC_STRING("Creates new instance of IReactNotificationService") |
Member
There was a problem hiding this comment.
Suggested change
| DOC_STRING("Creates new instance of IReactNotificationService") | |
| DOC_STRING("Creates a new instance of @IReactNotificationService") |
| "The `notificationName` must not be null.\n" | ||
| "The `sender` is the object that sends notification. It can be null.\n" | ||
| "The `data` is the data associated with the notification. It can be null.\n" | ||
| "Consider using @IReactPropertyBag for sending semi-structured data. It can be created " |
Member
There was a problem hiding this comment.
what does semi-structured mean in this context?
Comment on lines
+79
to
+82
| "The `notificationName` must not be null.\n" | ||
| "The `sender` is the object that sends notification. It can be null.\n" | ||
| "The `data` is the data associated with the notification. It can be null.\n" | ||
| "Consider using @IReactPropertyBag for sending semi-structured data. It can be created " |
Member
There was a problem hiding this comment.
"`notificationName` must not be null.\n"
"`sender` is the object that sends the notification. It can be null.\n"
"`data` is the data associated with the notification. It can be null.\n"
"Consider using @IReactPropertyBag for sending semi-structured data. It can be created "
| DOC_STRING( | ||
| "The data sent with the notification. It can be any WinRT type. " | ||
| "Consider using @IReactPropertyBag for semi-structured data. " | ||
| "It can be null if notification has no data. ") |
Member
There was a problem hiding this comment.
Suggested change
| "It can be null if notification has no data. ") | |
| "It can be null if the notification has no data associated with it. " |
Comment on lines
+33
to
+36
| "Because of the multi-threaded nature of the notifications, the handler can be still called " | ||
| "after the @.Unsubscribe method called if the @.IsSubscribed property is already checked. " | ||
| "Consider calling the @.Unsubscribe method and the handler in the same @IReactDispatcher " | ||
| "to ensure that no handler is invoked after the @.Unsubscribe method call.") |
Member
There was a problem hiding this comment.
Unclear what the "IsSubscribed property is already checked" means. Is it this? Also, by handler do you mean the instance of ReactNotificationHandler passed in to Subscribe?
Suggested change
| "Because of the multi-threaded nature of the notifications, the handler can be still called " | |
| "after the @.Unsubscribe method called if the @.IsSubscribed property is already checked. " | |
| "Consider calling the @.Unsubscribe method and the handler in the same @IReactDispatcher " | |
| "to ensure that no handler is invoked after the @.Unsubscribe method call.") | |
| "Because of the multi-threaded nature of notifications, the handler can be still called " | |
| "after the @.Unsubscribe method has been called (e.g. if the @.IsSubscribed property has already been checked). " | |
| "Consider calling the @.Unsubscribe method and the @ReactNotificationHandler on the same @IReactDispatcher " | |
| "to ensure that no handler is invoked after the @.Unsubscribe method call.") |
Member
Author
|
@asklar, the change was mostly about formatting/indenting the existing doc comments. I had to add new property in this IDL file in PR #6877 and it pained me to see bad formatting/indentation in this file. I appreciate your feedback on this PR and I will incorporate it in another PR that is going to target the content changes. My PRs in the last two days were mostly to provide scoped trivial changes before the main PR #6877 to fix the bug in the ReactNotificationService. |
vmoroz
added a commit
that referenced
this pull request
Jan 15, 2021
* 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)
ghost
pushed a commit
that referenced
this pull request
Mar 18, 2021
* Changes to ReactInstanceSettings.idl docs * Address PR feedback from @asklar * Add doc comments to IReactContext.idl * Add doc comments to IReactPropertyBag.idl * Address PR feedback and fix compilation issues * Change files * Format doc comments in IReactDispatcher.idl * Apply PR #6875 feedback to IReactNotificationService.idl * Format comments in IJSValueReader and IJSValueWriter * Update docs and formatting in remaining IDL files * Address PR feedback from @asklar
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
In this PR we update the doc comments in
IReactNotificationService.idl.There are no any functional changes.
Microsoft Reviewers: Open in CodeFlow