-
Notifications
You must be signed in to change notification settings - Fork 45
breaking: refactor network status interface #368
Conversation
Update. Let's do breaking changes, however, we need to update docs and sample app |
client.networkStatus.onStatusChangeListener(listener); | ||
|
||
return function cleanup() { | ||
// @ts-ignore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment can probably be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 for proposed renaming of onStatusChangeListener
and changes to the NetworkStatusChangeCallback
type
Needs fixes to the tests |
Like that we do get now this simple callback. Good to merge |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks so much for this. It's a great improvement!
Does this tweak how react-native's network status is setup? Working through what the new structure should be... |
I'm pretty sure this won't ever work: |
@Vednus We really apologize for the regression that happened in the mentioned PR. We are going to look at this and for the issue |
@kingsleyzissou we can test this with the react native sample app we provide |
@wtrocki no worries. Thanks for dealing with this and creating such a great library. |
Thanks! You are the Zissou! |
Proposed change
Currently event listeners are added to the
WebNetworkStatus
andCordovaNetworkStatus
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:
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:Suggested api changes
This is open for discussion, but possibly a better api would be
addListener
instead ofonStatusChangeListener
. And instead of passing an object with a function, the function could be added directly. This would mean changing theNetworkStatusChangeCallback
from an interface to a function type. However, these would be breaking changes.