breaking: refactor network status interface

[kingsleyzissou]

Proposed change

Currently event listeners are added to the WebNetworkStatus and CordovaNetworkStatus classes. The issue with this is that it offers no flexibility, and we keep adding listeners without the ability for removing them at a later stage.

For now the proposed changes are only slightly breaking, as the api has been kept the same. However, instead of a new event listener being added every time, the event listeners are created on instantiated and added to the window that trigger a handleNetworkStatusChange method. The responsibility of this method is to loop through the array of listeners and apply the result of the status change.

Code example:

const listener = {
      onStatusChange(networkInfo: NetworkInfo) {
        self.online = networkInfo.online;
        if (self.online) {
          queue.forwardOperations();
        }
      }
    };

this.networkStatus.onStatusChangeListener(listener);

This approach allows us to add and remove listeners more easily, which is required, for example in the cleanup of the React useNetworkStatus hook. eg:

this.networkStatus.removeListener(listener);

Suggested api changes

This is open for discussion, but possibly a better api would be addListener instead of onStatusChangeListener. And instead of passing an object with a function, the function could be added directly. This would mean changing the NetworkStatusChangeCallback from an interface to a function type. However, these would be breaking changes.

[kingsleyzissou]

CC @darahayes @wtrocki

[wtrocki]

Update. Let's do breaking changes, however, we need to update docs and sample app

[darahayes]

This comment can probably be removed

[darahayes]

+1 for proposed renaming of onStatusChangeListener and changes to the NetworkStatusChangeCallback type

[darahayes]

Needs fixes to the tests

[wtrocki]

Like that we do get now this simple callback. Good to merge

[darahayes]

Thanks so much for this. It's a great improvement!

[Vednus]

Does this tweak how react-native's network status is setup? Working through what the new structure should be...

[Vednus]

I'm pretty sure this won't ever work:
https://github.com/aerogear/offix/pull/368/files#diff-0e4b30e4ba2267970563a470c6c7684aR15
Also, in react native at least, mutations are firing multiple times once coming back online for me unless I only allow one listener.

[wtrocki]

@Vednus We really apologize for the regression that happened in the mentioned PR. We are going to look at this and for the issue

[wtrocki]

@kingsleyzissou we can test this with the react native sample app we provide

[kingsleyzissou]

@wtrocki @Vednus I've created an issue for this. We parked the React Native example app because of the Datasync-starter. But I will pick that up and investigate.

Here's the issue #401

[Vednus]

@wtrocki no worries. Thanks for dealing with this and creating such a great library.

[kingsleyzissou]

@Vednus, there's an example in the repo now for a network listener for React Native. There still seems to be an issue with the duplicate mutation. I've created a new issue for that (#408) and looking into it

[Vednus]

Thanks! You are the Zissou!