This differential uses CommIOSnotifications in AppDelegate notification - related callbacks and in JavaScript to request and validate notification permissions.
Details
Details
Build the app, after uninstalling it. Application should: ask for permission, receive foreground notifications, background notifications and correctly rescind when read on another machine.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| native/ios/Comm/AppDelegate.mm | ||
|---|---|---|
| 176 ↗ | (On Diff #20222) | Is this log useful without any other details? |
| native/ios/Comm/AppDelegate.mm | ||
|---|---|---|
| 176 ↗ | (On Diff #20222) | This is a debugging artifact that I accidentally forgot to remove. It is my flaw I frequently forget to remove logging I used for debugging. |
Comment Actions
Register for events sent from CommIOSNotifications in push-handler. This change greatly broadens the scope of this differential but this changes must be included in this diff. When we replace RNNotifications with CommIOSNotifications in AppDelegate, then AppDelegate callbacks send notifications to
NotificationCenter that CommIOSNotifications is observing. CommIOSNotifications reacts to observed notifications by sending relevant events to JavaScript. If we don't register in JavaScript for events that CommmIOSNotifications is sending in this differential, then this differential would leave app in
a state in which app for example does not know whether push permissions are enabled (whihc would result in inaccuratte UI).
| native/push/push-handler.react.js | ||
|---|---|---|
| 115–121 ↗ | (On Diff #20500) | Maybe we can move this to a separate file? We usually keep native modules / EventEmitters in a separate file for hygiene |
| 117 ↗ | (On Diff #20500) | We should avoid using Object... it's basically any |
| 122 ↗ | (On Diff #20500) | This should not be global scope |
| 148 ↗ | (On Diff #20500) | Why don't we just update registerPushPermissions to take the new params? I don't think we need all of these new lambdas |
| 503–509 ↗ | (On Diff #20500) | Not opposed, but curious why this needed to be added |
| native/push/push-handler.react.js | ||
|---|---|---|
| 503–509 ↗ | (On Diff #20500) | notification.getMessage() can possibly return null/undefined. This is how it was implemented in original code in our fork and it is how I implemented it in comm-ios-notification file. Therefore if the if statement that checks for title does not get executed body is possibly nullish and showInAppNotification type checking fails since it expects string. This is the simplest workaround around this problem. On one hand a possible solution might be to create several flow types/classes for different types of notifications. In this particular case we know for sure that this notification contains message since it is not rescind, so having separate types for regular notifications and rescinds could remove this if statement (type signatures for getMessage() could be different). On the other hand this is the only place in the code where we suffer from having one type for all notifications and still we can get a simple workaround. So I decided to apply simplest solution and not introduce additional complexity, but I am open to implement several flow types for different notifications if any reviewer disagrees. |
| native/push/push-handler.react.js | ||
|---|---|---|
| 503–509 ↗ | (On Diff #20500) | Makes sense, thanks for clarifying |
Comment Actions
Remove overriden didReceiveLocalNotifications method. We do not use local notifications in our App.
| native/push/ios.js | ||
|---|---|---|
| 29 ↗ | (On Diff #21433) | I'm not sure, but it seems like you have all of the methods now, so you could potentially remove the .... Not that important though... the type isn't wrong today, just might not be as specific as it could be |
| native/push/ios.js | ||
|---|---|---|
| 29 ↗ | (On Diff #21433) | This is an example of method present in native modules on iOS that we do not override (since we don't use it): https://reactnative.dev/docs/native-modules-ios#threading. I guess that native modules contain many methods that can (bo do not have to) be overridden. Since we don't override them, we don't include them in type specification. Therefore as far as I understand inexact type should stay here. |
| native/push/ios.js | ||
|---|---|---|
| 29 ↗ | (On Diff #21433) | That seems like an Objective-C method. Are there any JS methods that aren't typed here? You can figure this out by adding a debugger line and inspecting the object to see what methods it has |
| native/push/ios.js | ||
|---|---|---|
| 29 ↗ | (On Diff #21433) | console.log(Object.keys(CommIOSNotifications)) returns: ["requestPermissions", "completeNotif", "setBadgesCount", "consumeBackgroundQueue", "checkPermissions", "removeAllDeliveredNotifications", "removeDeliveredNotifications", "getDeliveredNotifications", "addListener", "removeListeners", "FETCH_RESULT_NO_DATA", "FETCH_RESULT_NEW_DATA", "FETCH_RESULT_FAILED", "getConstants"] So I conclude we can actually make this type exact (the same for Android). I will add subtask to ENG-2334 follow-ups. |
| native/push/ios.js | ||
|---|---|---|
| 29 ↗ | (On Diff #21433) | Nothing has been done here. Can you at least create the follow-up task and link here? |
| native/push/ios.js | ||
|---|---|---|
| 29 ↗ | (On Diff #21433) | |