This differential introduces CommIOSNotification class to JavaScript. It is equivalent of IOSNotification class from previously used Ashoat's fork. We need this class since it parses structure of raw Apple notification into more meaningful JavaScript object we rely on in various callbacks in
push-handler.
Details
Details
Flow and Eslint do not complain. Very next differential will use this class in JavaScript callbacks to various events send from native code.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
This class implementation is copied from Ashoat's fork https://github.com/Ashoat/react-native-notifications/blob/master/notification.ios.js. Calls to NotificationsIOS are replaced with equivalent calls to CommIOSNotifications
Comment Actions
Copy-paste should be avoided – all code should be written by you (with existing code as inspiration) and unused code should not be included
Comment Actions
Get rid of unused fields. Introduce RawIOSNotification type to avoid Object typing in NativeEventEmitter
Comment Actions
After quick private discussion I came to a conclusion that inexact type is actually not much better than Object so planning changes to create detailed types for all notifications.
Comment Actions
I disagree, I think inexact types are way better than Object (which is basically any). Inexact types are a great way to type something when you don't want to include everything – we use them a lot in libdefs and other places we are supporting third-party code.
That said – in this case, I think it would be best to remove unused properties rather than to use inexact types. If we have a field we don't need to type, that indicates we aren't using that field, and so we can probably just delete it.
Comment Actions
Now that we parse notification in Objective - C we can simplify this code and type notifications directly.
| native/push/comm-ios-notification.js | ||
|---|---|---|
| 8–15 ↗ | (On Diff #21010) | Let's go through these and make sure things are only nullable if they need to |
Comment Actions
Please make sure that the nullability is updated.
| native/push/comm-ios-notification.js | ||
|---|---|---|
| 19–20 ↗ | (On Diff #21010) | I don't think we have a convention of starting member variables with underscore. E.g. ThreadWatcher doesn't follow it. |
| 39–40 ↗ | (On Diff #21010) | It might be safer to set remoteNotificationCompleteCallbackCalled after successful completeNotif call. |
| native/push/comm-ios-notification.js | ||
|---|---|---|
| 9 ↗ | (On Diff #21041) | Can we add some comments explaining what these fields are? Especially the difference between id and identifier |
| native/push/comm-ios-notification.js | ||
|---|---|---|
| 7 ↗ | (On Diff #21041) | Nit |
| native/push/comm-ios-notification.js | ||
|---|---|---|
| 9 ↗ | (On Diff #21041) | This is my oversight. identifier property should not be included in this type. Ii should be only in CoreIOSNotificationDataWithRequestIdentifier. I hope the comment above CoreIOSNotificationDataWithRequestIdentifier definition is comprehensive enough to explain the difference. |
Comment Actions
Remove mistakenly introduced identifier field from CoreIOSNotificationData type definition.
| native/push/comm-ios-notification.js | ||
|---|---|---|
| 9 | This is notification id put by keyserver code in send.js | |
| 10 | This field is obtained from alert key from notification dictionary in Obj-C. According to https://developer.apple.com/documentation/usernotifications/setting_up_a_remote_notification_server/generating_a_remote_notification?language=objc, this is displayable text for the notification. body is the actual content of the message - value under key text in our messageInfos. title is the title of the notification - I think it is the name of the user that sent the message. Apple suggests in those docs that title is a short string immediately recognized by the user. | |