feat: add useNetInfoInstance - a non singleton-state hook to manage configs / events independently

[aiden-petersen]

Overview

The objective here is to allow consumption of this library without the global singleton. I am currently experiencing a bug where this library is continuing to check the network status in the background and then when my app comes back into foreground and I retrieve the state from netinfo, it's saying there's no connection. This is because the netinfo singleton is running in the background and the network request eventually gets blocked by iOS. see issue Issue #669. It would be nice to pause checking the status when ever my app goes into the background. ie useNetInfo({isPaused: true}). It would nice for all network checking to stop if useNetInfo is unmounted.

This library manages a singleton which is beneficial in some cases but not ideal in others:

• When you'd like a different config for each useNetInfo. Why does passing a config to useNetInfo need to cause all other hooks to break? When ever useNetInfo is called with a config, the existing singleton is torn down and other hooks will stop receiving updates.
• You'd like to stop checking the network info when in background for example. ATM there's no way to stop the singleton from running once it has started. When useNetInfo is unmounted, the singleton continues to run, when useNetInfo is re-mounted, it just taps back into the currently running singleton.
• Shared state across useNetInfo hooks is not following reacts principle of Hooks let you share stateful logic, not state itself. The example in the docs even mention a hook called useOnlineStatus

This is a patch we've made in our own app to solve Issue #669. I'm sure there are others out there who'd be keen on this functionality too so I'm starting an initial PR to see if there is any appetite for this or something else that'll achieve similar outcomes.

Test Plan

[mikehardy]

Very interesting - these seems fine (and like a nice fix for the issue you link)
Do you think this qualifies as a breaking change? You've maintained the old implementation for people that really need compatibility (for whatever reason) but as a new name - what I'm not sure about is if there is some case where the new implementation under the old name will actually change functionality for existing people, or if it will simply behave better in almost every way

Any answer is correct - please just let me know - the only difference is whether the release gets a semver major with a breaking changes note that people needing <insert very specific behavior criteria here from old implementation> should use the useNetInfoSingleton API name or whether it is just a semver minor

[aiden-petersen]

Very interesting - these seems fine (and like a nice fix for the issue you link) Do you think this qualifies as a breaking change? You've maintained the old implementation for people that really need compatibility (for whatever reason) but as a new name - what I'm not sure about is if there is some case where the new implementation under the old name will actually change functionality for existing people, or if it will simply behave better in almost every way

Any answer is correct - please just let me know - the only difference is whether the release gets a semver major with a breaking changes note that people needing <insert very specific behavior criteria here from old implementation> should use the useNetInfoSingleton API name or whether it is just a semver minor

Yeah good point thanks, I think it could definitely break peoples existing usage of it. It's probably best to stay on the safer side and keep existing behaviour for people and create a newly named hook for this functionality. The naming isn't obvious to me all I got is useNetInfoNonSingleton, useNetInfoInstance, useNetInfoV2?

I'd also like to add the ability to pause the hook, so something like this:

export function useNetInfo(isPaused = false, configuration?: Partial<Types.NetInfoConfiguration>) {
  const [networkInfoManager, setNetworkInfoManager] = useState<State>();
  const [netInfo, setNetInfo] = useState<Types.NetInfoState>({
    type: Types.NetInfoStateType.unknown,
    isConnected: null,
    isInternetReachable: null,
    details: null,
  });

  useEffect(() => {
    if (isPaused){
      return;
    }
    const config = {
      ...DEFAULT_CONFIGURATION,
      ...configuration,
    };
    const state = new State(config);
    setNetworkInfoManager(state);
    state.add(setNetInfo);
    return state.tearDown;
  }, [isPaused, configuration]);

  const refresh = useCallback(() => {
    networkInfoManager && networkInfoManager._fetchCurrentState();
  }, [networkInfoManager]);

  return {
    netInfo,
    refresh,
  };
}

[mikehardy]

useNetInfoInstance seems as good as anything? Naming is impossible - but that seems good enough to me. If it's not perturbing existing functionality, the sky's the limit on implementation really

[aiden-petersen]

useNetInfoInstance seems as good as anything? Naming is impossible - but that seems good enough to me. If it's not perturbing existing functionality, the sky's the limit on implementation really

Cool, have updated the PR. Thanks for reviewing

[mikehardy]

Only quibble now is lack of documentation, so this capability (and the pro and con of using it) won't be discoverable for people at all. Writing docs is no fun but as motivation, the more people using your code, the more people to find+fix things they see before you even see them, resulting in the code working better for you as well. Vs you being solo in this code path :-)

If you could maybe take the second two stanzas here (subscribe/get) and put them under a new "Global shared state style" header (or whatever sounds good to you for naming) then add a new "Hook-managed state style" header (or, whatever sounds good for naming) then describe the new instance hook and how + why you would use it?

It occurs to me that the useNetInfo existing hook has no documentation at all so maybe adding a quick mention up in the global shared style subscribe section...

[aiden-petersen]

Only quibble now is lack of documentation, so this capability (and the pro and con of using it) won't be discoverable for people at all. Writing docs is no fun but as motivation, the more people using your code, the more people to find+fix things they see before you even see them, resulting in the code working better for you as well. Vs you being solo in this code path :-)

If you could maybe take the second two stanzas here (subscribe/get) and put them under a new "Global shared state style" header (or whatever sounds good to you for naming) then add a new "Hook-managed state style" header (or, whatever sounds good for naming) then describe the new instance hook and how + why you would use it?

It occurs to me that the useNetInfo existing hook has no documentation at all so maybe adding a quick mention up in the global shared style subscribe section...

Thanks @mikehardy, I've just pushed the doc changes, let me know what you think

[mikehardy]

The documentation addition looks amazing! Seriously, that's a major improvement. Thank you

[matt-oakes]

🎉 This PR is included in version 11.1.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀