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 inconsistent is connected props #136

Closed
wants to merge 2 commits into from
Closed

Fix inconsistent is connected props #136

wants to merge 2 commits into from

Conversation

richardtks
Copy link

Motivation

When the checkConnectionInterval is set less than 1000 in withNetworkConnectivity, isConnected props keep changing between true and false, causing the re-render of the components.

Proposed solution

Introduced a props called nrAttempt where user can specify the number of attempt to check for connectivity until confirm the network status is offline. The default value of attempt is 3, user are allowed to change it according to their needs.

Changes:

  1. Added nrAttempt to the withNetworkConnectivity
  2. Changed the setTimeout in makeHttpRequest.js to XMLHttpRequest.ontimeout

@rgommezz
Copy link
Owner

Hi @richardtks, many thanks for the PR!

Before taking a look at the code, I have some questions:

Is this meant to fix #86?

When the checkConnectionInterval is set less than 1000 in withNetworkConnectivity, isConnected props keep changing between true and false, causing the re-render of the components.

How have you come to that realisation? I am trying to understand what's the underlying problem. Your proposed solution doesn't seem to easily map to what you explain in the motivation section.

@richardtks
Copy link
Author

@rgommezz, sorry, I missed some details, actually there are two fix, one is introduce the nrAttempt, as it could be sometime the result could be the false result, hence, introduce the nrAttempt can let the result to be more accurate, another fix is using the built-in timeout function of XMLHttpRequest instead of setTimeout in makeHttpRequest, the reason is that setTimeout is not executed in parallel, all the timeout logic will be executed at one time, causing the isConnected become false even though it has the access to the internet. Note that, onload, onerror, ontimeout are not executed in parallel as well but at least they have queue up mechanism.

for etc,
tOut with id 1 created.
tOut with id 2 created.
tOut with id 3 created.
tOut with id 4 created .
tOut with id 5 created.

id 1 successful.
id 2 successful.
id 4 successful.


id 3 timeout.
id 5 timeout.
(all the timeout will be executed at once)

You can read more about timeout function in https://softwareengineering.stackexchange.com/questions/314773/does-settimeout-really-execute-in-parallel

@rgommezz
Copy link
Owner

@richardtks thanks for the explanation. It makes sense that the native xhr timeout is way more reliable that a custom one.

I am currently undergoing a full refactor of the library, with the goal of releasing a new major version, v4.0 in the next days.

That means that unfortunately I can't merge this PR at the moment due to the potential conflicts. Since I want to credit your work, I'll add you as contributor for the time being.

Let's revisit this once 4.0 is out, in order to discuss the nAttempts functionality with more details.

@rgommezz rgommezz closed this Dec 24, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants