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

Add support for new Android NetInfo connectivity monitoring #22981

Closed
wants to merge 5 commits into from

Conversation

matt-oakes
Copy link
Contributor

@matt-oakes matt-oakes commented Jan 14, 2019

This PR changes NetInfo to use the newer and better supported NetworkCallback on API levels 24 and above.

This is in place of the now deprecated BroadcastReceiver method. Both are kept so we can continue supporting down to API level 16. This is done by splitting out the connectivity code into two different classes, organised around a base abstract class to avoid duplication. Which class to use is chosen at runtime with a simple if statement in the NetInfo module.

The structure of the connectionChange event has not been changed, so this will be a non-breaking change for most users. The slight exception to that is that I have now removed the previously deprecated code from the module to avoid needing to support it in the new NetworkCllback code, however, this should be a non-issue as it was deprecated 2 years ago and doesn't even feature in the documentation anymore.

There is a slight difference in behaviour, which is down to a limitation in the old broadcast receiver method of updating the connectivity status. If the device changes from one cellular network type to another (effective network type), for example from 2g to 4g, this previously didn't trigger a new event. Using the new Network Callback method, it does.

Changelog:

[Android] [Changed] - NetInfo on API level 24+ (N) we now use NetworkCallback to receive updates about the network status
[General] [Removed] - Removed the previously deprecated change event from the NetInfo module

Test Plan:

  • Open the RNTester app to the NetInfoExample screen.
  • Toggle the Wifi on and off.
  • Change the network type in the emulator (changing from LTE to GPRS for example).
  • See that the events are fired and have the correct status in the RNTester screen.
  • Do this for devices above and below the API level 24 threshold to ensure it works the same on both (aside from the limitation on changes to the effective network type for devices below API level 24, see above).

Gif of it working on Android:
ezgif com-video-to-gif

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Tests This PR adds or edits a test case. labels Jan 14, 2019
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.

@pull-bot

This comment has been minimized.

@matt-oakes matt-oakes changed the title Android: NetInfo Add support for new Android NetInfo connectivity monitoring Jan 14, 2019
@react-native-bot react-native-bot added 🌐Networking Related to a networking API. 🔶APIs API: NetInfo Platform: Android Android applications. labels Jan 14, 2019
@matt-oakes
Copy link
Contributor Author

Rebased this on master to resolve the conflicts from 5a87093.

All good to review again 👍

@ngandhy
Copy link
Contributor

ngandhy commented Feb 3, 2019

Hey @matt-oakes did you run across the issues in #22966 (and parts of #8615)? I'm hoping maybe you came across that and fixed it :) . Might help expedite this hitting master

Basically that the connectionChange callback doesn't reliably fire. (e.g - if I start connected, then disconnect (get callback) and then connect again (no callback).

Thanks!

@matt-oakes
Copy link
Contributor Author

@ngandhy This PR mainly touches the Android side of that module and leaves the iOS side alone aside from removing the long deprecated API.

With the Lean Core initiative (react-native-community/discussions-and-proposals#6) I would be happy to help move out and maintain this part of the code, however, the exact process for doing that is currently not 100% clear. It should, however, allow fixes and changes like this to move forward.

@cpojer
Copy link
Contributor

cpojer commented Feb 7, 2019

@matt-oakes given that we are splitting NetInfo out into its own repo, I'm gonna close this PR here and we can discuss the same change on that other repo :)

@matt-oakes
Copy link
Contributor Author

Opened the PR in the new repository 👍

@matt-oakes matt-oakes deleted the netinfo branch February 8, 2019 16:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: NetInfo CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. 🌐Networking Related to a networking API. Platform: Android Android applications. Tests This PR adds or edits a test case.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants