chore: improve wifi state management #220
Conversation
We need the device's wifi state at a few points in the app. The way we previously did this had a few disadvantages: 1. If you were connected to wifi and another network (like cellular), we might get the wrong network's IP address. You might also be unable to fetch the SSID. 2. It used two different dependencies, because neither does exactly what we want. 3. These dependencies rely on deprecated APIs. This change fixes those by implementing a native module, `WifiModule`, to address these problems. It also adds an React hook for using it. I think this is a useful change on its own, but will also help us as we try to [improve local peer discovery][0] in upcoming work. [0]: digidem/comapeo-core#474
EvanHahn
force-pushed
the
wifi-improvements
branch
from
1fdcb39 to
8a2b459
Compare
| // here, | ||
| // for example: | ||
| // add(MyReactNativePackage()) | ||
| add(WifiPackage()) |
There was a problem hiding this comment.
See the relevant React Native docs.
| @@ -0,0 +1,16 @@ | |||
| package com.comapeo | |||
There was a problem hiding this comment.
See the relevant React Native docs.
| @ReactMethod | ||
| fun addListener(eventName: String) { | ||
| if (numberOfListeners == 0) { | ||
| startListening() | ||
| } | ||
| numberOfListeners++ | ||
| } | ||
|
|
||
| @ReactMethod | ||
| fun removeListeners(count: Int) { | ||
| numberOfListeners -= count | ||
| if (numberOfListeners == 0) { | ||
| stopListening() | ||
| } | ||
| } |
There was a problem hiding this comment.
See the relevant React Native docs for sending events from a native module.
There was a problem hiding this comment.
Generally, ok, however I'm concerned about the increased maintenance overhead here. We also will want to know whether wifi is turned on or off, and in the future will likely want to add signal strength and whether the internet is connected, all of which is returned by @react-native-community/netinfo. Also we will need iOS support this year.
I know the netinfo module above emits events about the current connected interface, rather than the wifi interface, which is why with the current usage we always request the wifi state when the network connectivity changes, and we also poll wifi state. This isn't ideal, but is it a huge issue? It would obviously be nicer to correctly register the change listener to the wifi interface like you do here, so we don't need to poll, but we also take on a lot of other maintenance and bug fixing for edge-case devices. Is there a bug in the way the netinfo module returns the SSID or IP address when calling NetInfo.fetch('wifi')? Maybe better to submit a PR to the netinfo module, or even just maintain a patch, rather than write and maintain our own wifi module, given that netinfo is fairly mature and actively maintained.
| throw new Error('Invalid wifi state from native module'); | ||
| }; | ||
|
|
||
| export const useWifiState = () => { |
There was a problem hiding this comment.
This needs a request for the current state when the hook is first initialized - if there is no change event then the state will remain null.
|
You're right. I wrongly assumed that Closing this PR. |
We need the device's wifi state at a few points in the app. The way we previously did this had a few disadvantages:
If you were connected to wifi and another network (like cellular), we might get the wrong network's IP address. You might also be unable to fetch the SSID.
It used two different dependencies, because neither does exactly what we want.
These dependencies rely on deprecated APIs.
This change fixes those by implementing a native module,
WifiModule, to address these problems. It also adds an React hook for using it.Here's what it looks like in the app:
I think this is a useful change on its own, but will also help us as we try to improve local peer discovery in upcoming work.