Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue when using redux and checkIntervalOfflineOnly #133

Merged
merged 3 commits into from
Dec 10, 2018

Conversation

usrbowe
Copy link
Contributor

@usrbowe usrbowe commented Nov 29, 2018

What this PR fixes

If using redux integration and option checkIntervalOfflineOnly.
It will never make request to check internet connection.
HOC withNetworkConnectivity has internal state, in which connection status is stored.
This however never gets updated. It's only set once during initialisation.

Test scenario

Make sure there is no internet connection

withNetworkConnectivity({
  withRedux: true,
  checkConnectionInterval: 3000,
  checkIntervalOfflineOnly: true,
}),

Open app and check if there is any network request made to google.com. There will be no requests made. If we turn off option checkIntervalOfflineOnly, everything works fine.
With my change, it will always sync internal state with external redux state.

If using redux integration with combination of `checkIntervalOfflineOnly` option. It will never make request, since default `isConntected` is true, after that there is no point of synchronization with redux state.
Copy link
Owner

@rgommezz rgommezz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks very much for this, great catch! Just some stylistic nits before merging.

@@ -83,7 +83,12 @@ const withNetworkConnectivity = ({
}

setupConnectivityCheckInterval(() => {
if (this.state.isConnected && checkIntervalOfflineOnly) {
let isConnected = this.state.isConnected;
Copy link
Owner

@rgommezz rgommezz Dec 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please destructure the store to avoid that long property access?

const { store } = this.context;
if (withRedux && store) {
	isConnected = store.getState().network.isConnected;
}

...

Also, could you also rephrase the comment to "When using redux integration, the connection state is coming from the store"

Thanks!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, make sense 👍

@rgommezz rgommezz merged commit fc73028 into rgommezz:master Dec 10, 2018
@rgommezz
Copy link
Owner

Thanks very much @usrbowe!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
2 participants