-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Incorporate hasPendingNetworkCheck #44565
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -112,6 +112,7 @@ function subscribeToNetInfo(): () => void { | |
reachabilityUrl: `${CONFIG.EXPENSIFY.DEFAULT_API_ROOT}api/Ping?accountID=${accountID || 'unknown'}`, | ||
reachabilityMethod: 'GET', | ||
reachabilityTest: (response) => { | ||
hasPendingNetworkCheck = false; | ||
if (!response.ok) { | ||
return Promise.resolve(false); | ||
} | ||
|
@@ -123,6 +124,13 @@ function subscribeToNetInfo(): () => void { | |
|
||
// If a check is taking longer than this time we're considered offline | ||
reachabilityRequestTimeout: CONST.NETWORK.MAX_PENDING_TIME_MS, | ||
reachabilityShouldRun: () => { | ||
if (!hasPendingNetworkCheck) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess we can simplify this condition a bit: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. tbh I find the initial implementation provided easier to follow (less cognitively demanding). What I'd think about though is limiting the nesting further more by introducing an early return (reverting the boolean check). reachabilityShouldRun: () => {
- if (!hasPendingNetworkCheck) {
- hasPendingNetworkCheck = true;
- return true;
- }
- return false;
+ if (hasPendingNetworkCheck) {
+ return false;
+ }
+
+ hasPendingNetworkCheck = true;
+ return true;
}, |
||
hasPendingNetworkCheck = true; | ||
return true; | ||
} | ||
return false; | ||
}, | ||
}); | ||
} | ||
|
||
|
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.
I see this variable is used in
recheckNetworkConnection
... I think that's just a manual call to check for the connection, is that correct?And if so, why would we need to update this variable both here and in the current place we are updating it?
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.
Ah yeah you're right. I'll fix that since any check including NetInfo.refresh() will now check for the state first.
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.
Fixed this.
I kept
so that we only see the
[NetworkConnection] recheck NetInfo
Log when necessary. Otherwise, we would see the log every 60s. But I can remove it if we want to add the Log toreachabilityShouldRun
. The only problem with this is we won't be able to differentiate whether it came from the automatic or manual recheck.