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.

Sign up for GitHub

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

Jump to bottom

feat: reachability test may be enabled/disabled via user-supplied function #513

Merged
merged 6 commits into from Nov 7, 2021
Merged

feat: reachability test may be enabled/disabled via user-supplied function #513

merged 6 commits into from Nov 7, 2021

Conversation

YimingIsCOLD
Copy link
Contributor

Overview

There are multiple requests to disable internet reachability check. The proposed solution is to add a reachabilityShouldRun option to configure. This new option accepts a function that returns a boolean. This boolean flag will be used in the _setExpectsConnection function to determine whether _checkInternetReachability should be called.

Proposed Solution Reference: #178 (comment)

Test Plan

New set of unit tests are added to check if reachabilityShouldRun option is working correctly in both NetInfo.addEventListener and NetInfo.fetch.

Fixes: #178 #380 #440

YimingIsCOLD and others added 4 commits November 2, 2021 17:29
@YimingIsCOLD @wholesomewilson
feat: Add reachabilityShouldRun configuration
3d20703 
Co-authored-by: wholesomewilson <wilsonwan88@gmail.com>
@YimingIsCOLD @wholesomewilson
fix: eventListenerCallbacks and fetch spec
c51e3ea 
Co-authored-by: wholesomewilson <wholesomewilson@users.noreply.github.com>
@YimingIsCOLD @wholesomewilson
docs: Update README.md
c58eff0 
Co-authored-by: wholesomewilson <wilsonwan88@gmail.com>
@YimingIsCOLD @wholesomewilson
test: Add comment for fetch spec
ca9cd54 
Co-authored-by: wholesomewilson <wilsonwan88@gmail.com>
mikehardy
mikehardy reviewed Nov 3, 2021
View reviewed changes
README.md Outdated Show resolved Hide resolved
mikehardy
mikehardy requested changes Nov 3, 2021
View reviewed changes
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

This looks really fantastic to me. I had a couple comments I'm curious for the answers on but as I'm sure you see them, they're nitpicks vs the general idea and general level of the PR. Thanks for contributing this!

@YimingIsCOLD @wholesomewilson
docs: Update README.md
66f84f9 
Co-authored-by: wholesomewilson <wilsonwan88@gmail.com>
mikehardy
mikehardy approved these changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

The doc update looks great, I don't see a reason not to merge this, so here we go :-) - thanks!

@mikehardy mikehardy added the pending merge A PR that will be merged shortly, waiting for CI or final comment label Nov 5, 2021
@mikehardy
Copy link
Contributor

This may have a testing issue, I had to cancel the previous run:


Fri, 05 Nov 2021 13:32:57 GMT Test Suites: 6 passed, 6 total
Fri, 05 Nov 2021 13:32:57 GMT Tests:       81 passed, 81 total
Fri, 05 Nov 2021 13:32:57 GMT Snapshots:   0 total
Fri, 05 Nov 2021 13:32:57 GMT Time:        13.545 s
Fri, 05 Nov 2021 13:32:57 GMT Ran all test suites matching /\/src\//i in 3 projects.
Fri, 05 Nov 2021 13:32:58 GMT Jest did not exit one second after the test run has completed.
Fri, 05 Nov 2021 13:32:58 GMT
Fri, 05 Nov 2021 13:32:58 GMT This usually means that there are asynchronous operations that weren't stopped in your tests. Consider running Jest with `--detectOpenHandles` to troubleshoot this issue.
Fri, 05 Nov 2021 13:50:56 GMT Error: The operation was canceled.

Note the timestamps, it was sitting for 18 minutes where normally it's almost immediate

mikehardy
mikehardy requested changes Nov 5, 2021
View reviewed changes
Copy link
Contributor

@mikehardy mikehardy left a comment •
edited

Choose a reason for hiding this comment

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

Unfortunately I can't merge this yet - there is some interaction between the new test code and jest where something is not resolving, so the tests hang. This is not present on HEAD of default branch so it must be this PR somehow. Can you investigate? It looks so ready to go, too! 😩

https://github.com/react-native-netinfo/react-native-netinfo/runs/4117704891?check_suite_focus=true

@YimingIsCOLD
Copy link
Contributor Author

I think I have found the issue. It was the setTimeout in internetReachability.ts scheduling another _checkInternetReachability and cause jest to wait indefinitely. The fix is to use jest.useFakeTimers() to mock setTimeout so no more _checkInternetReachability will be scheduled.

mikehardy
mikehardy approved these changes Nov 7, 2021
View reviewed changes
Copy link
Contributor

@mikehardy mikehardy left a comment

Choose a reason for hiding this comment

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

It appears to be a thing of beauty now! ⚡

@mikehardy mikehardy changed the title Add reachability should run configuration feat: reachability test may be enabled/disabled via user-supplied function Nov 7, 2021
@mikehardy mikehardy removed the pending merge A PR that will be merged shortly, waiting for CI or final comment label Nov 7, 2021
@mikehardy mikehardy merged commit 83c1e0d into react-native-netinfo:master Nov 7, 2021
github-actions bot pushed a commit that referenced this pull request Nov 7, 2021
@semantic-release-bot
chore(release): 6.1.0 [skip ci]
b2761ec 
# [6.1.0](v6.0.6...v6.1.0) (2021-11-07)

### Features

* reachability test may be enabled/disabled via user-supplied function ([#513](#513)) ([83c1e0d](83c1e0d))
@matt-oakes
Copy link
Collaborator

🎉 This PR is included in version 6.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@mikehardy mikehardy mentioned this pull request Nov 13, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikehardy mikehardy mikehardy approved these changes

@matt-oakes matt-oakes Awaiting requested review from matt-oakes matt-oakes is a code owner

Assignees
No one assigned
Labels
released
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Impact on battery life caused by internet reachability test
3 participants
@YimingIsCOLD @mikehardy @matt-oakes