Fix #8615: NetInfo.isConnected for iOS #17397
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This is fixing react-native issue facebook#8615. The problem was that on initialization the _connectionType variable was still set to its default value of RCTConnectionTypeUnknown. The exposed API method did nothing to determine this unless a subscription had be established and then the method would simply return the last reported value. Instead, the exposed oneshot API call now actually checks the connection status through the same methods as the subscription and updates RCTNetInfo’s values before returning. In order to avoid reporting events without a subscription, a flag is set and unset on calls to startObserving and stopObserving.
Check Network on Request
Merge Dev
Previously, the one-time and subscription based APIs relyed on a class level `SCNetworkReachabilityRef` instance. However, closing after the one-shot caused the instance to no longer be usable. Now there is a general method, `getReachabilityRef`, to get a new reachability ref. The one-shot instance is closed directly after use but the subscription instance is stored for reuse in `stopObserving`.
Seperated SCNetworkReachabilityRef instances
facebook-github-bot
added
the
CLA Signed
alburdette619
changed the title
|
@alburdette619 I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project. |
|
@shergin I'm tagging you here because you were the last one to do a PR on NetInfo. I don't see any need to rebase currently since no changes have been made to the only file here since last year. This is my first time attempting to contribute to RN, please let me know if anything more is needed from me. Thanks! |
Merge in current master 02-19-2018
|
Trying to find a reviewer and taking anyone off of this file's last PR I can @christopherdro @hramos. |
facebook-github-bot
added
the
Import Started
facebook-github-bot
approved these changes
Feb 27, 2018
There was a problem hiding this comment.
@hramos is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
facebook-github-bot
pushed a commit
that referenced
this pull request
Mar 19, 2018
Summary: The pull request that added this (#17397) simply forgot to remove the callback, which would cause crashes if the RCTNetInfo module was ever deallocated. While that usually doesn't happen in apps, it can if the user logs out and you need to wipe all the RCT modules (to remove user data, for instance). Reviewed By: PeteTheHeat Differential Revision: D7322999 fbshipit-source-id: e49ec7311b39920f7b7743a5854c0dda1dbccc73
hamaron
pushed a commit
to hamaron/react-native
that referenced
this pull request
Apr 9, 2018
Summary: The pull request that added this (facebook#17397) simply forgot to remove the callback, which would cause crashes if the RCTNetInfo module was ever deallocated. While that usually doesn't happen in apps, it can if the user logs out and you need to wipe all the RCT modules (to remove user data, for instance). Reviewed By: PeteTheHeat Differential Revision: D7322999 fbshipit-source-id: e49ec7311b39920f7b7743a5854c0dda1dbccc73
campsafari
pushed a commit
to exozet/react-native
that referenced
this pull request
Apr 11, 2018
Summary: The pull request that added this (facebook#17397) simply forgot to remove the callback, which would cause crashes if the RCTNetInfo module was ever deallocated. While that usually doesn't happen in apps, it can if the user logs out and you need to wipe all the RCT modules (to remove user data, for instance). Reviewed By: PeteTheHeat Differential Revision: D7322999 fbshipit-source-id: e49ec7311b39920f7b7743a5854c0dda1dbccc73
LukeDurrant
pushed a commit
to LukeDurrant/react-native
that referenced
this pull request
Apr 11, 2018
Summary: The pull request that added this (facebook#17397) simply forgot to remove the callback, which would cause crashes if the RCTNetInfo module was ever deallocated. While that usually doesn't happen in apps, it can if the user logs out and you need to wipe all the RCT modules (to remove user data, for instance). Reviewed By: PeteTheHeat Differential Revision: D7322999 fbshipit-source-id: e49ec7311b39920f7b7743a5854c0dda1dbccc73
LukeDurrant
pushed a commit
to LukeDurrant/react-native
that referenced
this pull request
Apr 11, 2018
Summary: The pull request that added this (facebook#17397) simply forgot to remove the callback, which would cause crashes if the RCTNetInfo module was ever deallocated. While that usually doesn't happen in apps, it can if the user logs out and you need to wipe all the RCT modules (to remove user data, for instance). Reviewed By: PeteTheHeat Differential Revision: D7322999 fbshipit-source-id: e49ec7311b39920f7b7743a5854c0dda1dbccc73
grabbou
pushed a commit
that referenced
this pull request
Apr 27, 2018
Summary: The pull request that added this (#17397) simply forgot to remove the callback, which would cause crashes if the RCTNetInfo module was ever deallocated. While that usually doesn't happen in apps, it can if the user logs out and you need to wipe all the RCT modules (to remove user data, for instance). Reviewed By: PeteTheHeat Differential Revision: D7322999 fbshipit-source-id: e49ec7311b39920f7b7743a5854c0dda1dbccc73
hamaron
pushed a commit
to hamaron/react-native
that referenced
this pull request
Jun 7, 2018
Summary: The pull request that added this (facebook#17397) simply forgot to remove the callback, which would cause crashes if the RCTNetInfo module was ever deallocated. While that usually doesn't happen in apps, it can if the user logs out and you need to wipe all the RCT modules (to remove user data, for instance). Reviewed By: PeteTheHeat Differential Revision: D7322999 fbshipit-source-id: e49ec7311b39920f7b7743a5854c0dda1dbccc73
macdoum1
pushed a commit
to macdoum1/react-native
that referenced
this pull request
Jun 28, 2018
Summary: The pull request that added this (facebook#17397) simply forgot to remove the callback, which would cause crashes if the RCTNetInfo module was ever deallocated. While that usually doesn't happen in apps, it can if the user logs out and you need to wipe all the RCT modules (to remove user data, for instance). Reviewed By: PeteTheHeat Differential Revision: D7322999 fbshipit-source-id: e49ec7311b39920f7b7743a5854c0dda1dbccc73
even-huang
pushed a commit
to ant-worker/react-native
that referenced
this pull request
Jul 26, 2018
Properly tear down the reachabilityRef in RCTNetInfo Summary: The pull request that added this (facebook#17397) simply forgot to remove the callback, which would cause crashes if the RCTNetInfo module was ever deallocated. While that usually doesn't happen in apps, it can if the user logs out and you need to wipe all the RCT modules (to remove user data, for instance).
matt-oakes
pushed a commit
to matt-oakes/react-native
that referenced
this pull request
Feb 7, 2019
Summary: This is fixing facebook#8615. The problem was that, on initialization, the `_connectionType` variable was still set to its default value of `RCTConnectionTypeUnknown`. The exposed API method did nothing to determine this unless a subscription had be established and then the method would simply return the last reported value. Instead, the exposed oneshot API call now actually checks the connection status through the same methods as the subscription and updates RCTNetInfo’s values before returning. In order to avoid reporting events without a subscription, a flag is set and unset on calls to start/stopObserving. - start app - observe the (in)correct reporting of the manual status - change network status to offline - press refresh - observe the manual fetch - start subscription - change network status to online - press refresh to show that the manual refresh works (only now working for current RN version) - change network status to offline - stop subscription - change network status to online - press refresh to show manual refresh does(n't) work without subscription - start subscription to show it updates to current Current Behavior: https://drive.google.com/file/d/1Ods6HORgp_vfm1mQVjGwhtH1D7issxjo/view?usp=sharing Fixed Behavior: https://drive.google.com/file/d/11H1UOF33LeMGvXEOoapU62ARDSb7qoYv/view?usp=sharing [IOS] [BUGFIX] [Libraries\Network\RCTNetInfo.m] - Fixed facebook#8615, `iOS: NetInfo.isConnected returns always false`, by decoupling the fetch from the status of the subscription. Closes facebook#17397 Differential Revision: D7102771 Pulled By: hramos fbshipit-source-id: ea11eb0b1a7ca641fc43da2fe172cf7b2597de4a
matt-oakes
pushed a commit
to matt-oakes/react-native
that referenced
this pull request
Feb 7, 2019
Summary: The pull request that added this (facebook#17397) simply forgot to remove the callback, which would cause crashes if the RCTNetInfo module was ever deallocated. While that usually doesn't happen in apps, it can if the user logs out and you need to wipe all the RCT modules (to remove user data, for instance). Reviewed By: PeteTheHeat Differential Revision: D7322999 fbshipit-source-id: e49ec7311b39920f7b7743a5854c0dda1dbccc73
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This is fixing #8615. The problem was that, on initialization, the
_connectionTypevariable was still set to its default value ofRCTConnectionTypeUnknown. The exposed API method did nothing to determine this unless a subscription had be established and then the method would simply return the last reported value. Instead, the exposed oneshot API call now actually checks the connection status through the same methods as the subscription and updates RCTNetInfo’s values before returning.In order to avoid reporting events without a subscription, a flag is set and unset on calls to start/stopObserving.
Test Plan
Current Behavior: https://drive.google.com/file/d/1Ods6HORgp_vfm1mQVjGwhtH1D7issxjo/view?usp=sharing
Fixed Behavior: https://drive.google.com/file/d/11H1UOF33LeMGvXEOoapU62ARDSb7qoYv/view?usp=sharing
Release Notes
[IOS] [BUGFIX] [Libraries\Network\RCTNetInfo.m] - Fixed #8615,
iOS: NetInfo.isConnected returns always false, by decoupling the fetch from the status of the subscription.