This differential introduces native module that will serve as a bridge between CommNotificationsHandler and JavaScript. CommNotificationsHAndler will send broadcasts when device token or foreground notifications arrive. CommAndroidNotificationsEventEmitter will receive those broadcasts and
send appropriate events to JavaScript. At this point it already supports receiving new device token event from CommNotificationsHandler and forwarding it to JavaScript. Using LocalBroadcastManager makes implementation efficient since it bypasses OS.
Details
Details
Build the app. Import the module somwhere in JS from NativeModules and call addListener/removrListener. App shouldn't crash.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
| native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsEventEmitter.java | ||
|---|---|---|
| 23 ↗ | (On Diff #20971) | The docs https://developer.android.com/reference/androidx/localbroadcastmanager/content/LocalBroadcastManager suggest that This class is deprecated. |
| 25–27 ↗ | (On Diff #20971) | Is it safe to register in the constructor? Are we sure that the object will be fully constructed before any message is received by the receiver? Are there some lifecycle methods which can be used instead to register a receiver? |
| 35–43 ↗ | (On Diff #20971) | Is it possible for these methods to be called from multiple threads? |
| 36–38 ↗ | (On Diff #20971) | This is confusing to me. Why do we ignore an event name? Also, why do we call addListener with event as an argument? Usually a pattern is that addListener accepts a listener (object / function / etc.) that gets called when an even happens. So at this point it isn't clear to me how this class will be used. |
| native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsEventEmitter.java | ||
|---|---|---|
| 35–43 ↗ | (On Diff #20971) | Implementing this method in this way is required by react-native: https://reactnative.dev/docs/native-modules-android#sending-events-to-javascript |
| 36–38 ↗ | (On Diff #20971) | This is required by react-native: https://reactnative.dev/docs/native-modules-android#sending-events-to-javascript |
| native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsEventEmitter.java | ||
|---|---|---|
| 36–38 ↗ | (On Diff #20971) | Yeah this is normal – these methods are for setup/teardown when JS subscribes/unsubscribes. We only have one kind of event, so the event name doesn't matter We should arguably be doing the setup in the constructor in addListener instead, but I'm not sure if there's something there that needs to be running even before addListener is called |
| native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsEventEmitter.java | ||
|---|---|---|
| 25–27 ↗ | (On Diff #20971) | I cannot guarantee that receiver will not receive message before this object is fully constructed. Bu on the other hand I can't see potential disadvantages of this fact. |
| native/android/app/src/main/java/app/comm/android/notifications/CommAndroidNotificationsEventEmitter.java | ||
|---|---|---|
| 19 ↗ | (On Diff #20971) | We can make it safer by making it volatile |
| 25–27 ↗ | (On Diff #20971) | In this case it might be ok, but if we have a subclass it is a potential NPE. But I don't think we will create any subclass of this class. |
| 35–43 ↗ | (On Diff #20971) | I wouldn't say it's required - it's rather suggested. Just to be safe, I'd make listenersCount volatile. |
Comment Actions
These methods are expected to be called from the JS thread only, but adding a volatile will not hurt