Add allowsCellularAccess flag for iOS fetch calls

[bondparkerbond]

Summary

On iOS in React Native, immediately after successfully connecting to a WiFi network, if 4G is also on, it is possible that network requests sent via fetch get sent over 4G instead of WiFi for several seconds (between 1 and 40+ seconds depending on the device and network in my experience). If the calls are meant to be communicating with a local Wireless Device (such as a wireless router, etc.), then API calls that get sent over 4G to the internet are lost. Note that Reachability/NetInfo are saying that we are connected to a WiFi network and are not sufficient to prevent this from happening. To prevent this, I would like to be able to set an instance property that iOS exposes for network requests allowsCellularAccess to NO to ensure a request is never sent over 4G.

Note:

This code needs to be able to accept a flag to allow the user to override the default value of YES, but only when they explicitly opt in to this decision. I am not sure the best place to set and pass in this value, so I created this issue, where it was recommended that I make a PR and discuss how best to do this here. Before this PR could be merged we would need to allow the NO to be configurable in some way.

Additional information about the allowsCellularAccess property can be found in the Apple documentation.

Changelog

[iOS] [Added] - Ability to force network requests to use WiFi using the allowsCellularAccess property. This can ensure that network requests are sent over WiFi if communicating with a local hardware device and is accomplished by setting a flag. Default behavior of allowing network connections over cellular networks when available is unchanged.

Test Plan

Happy Path 1a: No flag is set and the value allowsCellularAccess remains unchanged at YES.

Desired Behavior: fetch calls can still be sent over 4G

• Test that when no flag is passed in fetch calls can be sent over 3/4G on a device with WiFi toggled off.
• Test that network requests also work with Wifi and 4G on.
• Test that if 4G is off and WiFi is on that network calls still succeed over WiFi.
• If both WiFi and 4G are off, test that network calls would fail in the same way as they currently do.
• If possible to check the code, see that the allowsCellularAccess property is either not set, or is set to YES depending on how this is implemented.

Happy Path 1b: A user sets a flag for allowsCellularAccess to remain the default value YES.

Desired Behavior: fetch calls can still be sent over 4G

• Repeat the tests for Happy Path 1a with the same expected results.

Happy Path 2: A user sets a flag to change the allowsCellularAccess property to NO.

Desired Behavior: fetch calls can NOT be sent over 4G

• With only WiFi on and 4G off, ensure network requests still function as expected.
• With WiFi and 4G off, ensure that network calls do NOT succeed as currently expected.
• With WiFi off and 4G on, ensure that network calls do NOT succeed as if 4G was also off.
• With WiFi and 4G on, test that network calls to a local hardware device that is connected via WiFi succeed. (It may also be useful to have the same call able to interact with the local hardware device not connected to the internet(which the API call should hit) and to a remote API(which the API call should NOT hit) that can let us know if a request was sent over 4G when it should not have been.)
• If possible to check the code, see that the allowsCellularAccess property is set to NO. This would probably look like either:
  [configuration setAllowsCellularAccess:NO]; or configuration.allowsCellularAccess = NO; which are functionally equivalent and would probably be done inside RCTHTTPRequestHandler.mm

Handle User Error: Test that the user sets the flag in an incorrect way or with an invalid value.

Desired Behavior: fetch calls can still be sent over 4G

• Ensure that the allowsCellularAccess value remains at its default state of YES, repeat the tests for Happy Path 1a with the same expected results.
• If appropriate, check that we throw an error/warning so the user knows that they did not set the value correctly with information on how to correctly set the flag

Note:

This change should not affect UI and hence no screenshots or videos are included in this. Once the exact change gets finalized I'll create a PR for the docs to explain how to set this functionality.

[kelset]

Thanks for the PR!

@cpjoer do you know who would be able to help in giving feedback to this?

[bondparkerbond]

@cpojer Please let me know if there is any way I can help once you've identified someone who can help look into this.

[ericlewis]

@bondparkerbond I am happy to help! First of all, we probably should define or figure out what an actual api for this looks like, and see perhaps if there is an android equivalent. Because I think that the option for this would be some sort of global-ish flag, it should be important that android behavior reacts to this as well. I’ve had some ideas on how this could look:

• setting a global flag
• passing a special header to change behavior per request
• extending fetches capabilities itself

[bondparkerbond]

@ericlewis I would be ok with any of the above.

Implementing a cross platform fix may be difficult as the Networking library fetch uses is implemented differently on iOS and on Android. On iOS, it uses URLSession, but on Android it interfaces with the 3rd party OkHTTP so extending fetches capabilities on both platforms might be beyond the scope of this. Please note that my knowledge of this is mostly based on a Medium article from 2017 so it may be a little outdated.

Regarding Android specifically, there is a way to forceWifi on Android if you use the 3rd party package: react-native-android-wifi. It should be noted that this implementation only works on certain Android Versions and also requires some scary looking permissions on Android 6.0.0. It also can cause some bugs if it is set when you aren't connected to the WiFi you want to use when it is turned on, but that may just be an implementation detail that can be fixed.

I think my 1st choice would be being able to pass in an additional header that makes individual requests use WiFi, and I know that on iOS you can set the allowsCellularAccess property on the Session or on an individual request, so in theory we could configure the Network library to allow us to modify allowsCellularAccess on either/both of those. We could even expose additional properties beyond allowsCellularAccess if we wished, but I'm not sure which work on both both platforms.

My original thought when making this PR was that having a well documented way to force all API calls in a session to use WiFi would be valuable and would be significantly easier to implement. Since I have very limited experience with Java and Objective-C, and since iOS was the platform I was currently blocked on, I decided to propose a minimum viable fix that would add new functionality with a much lower risk of introducing new Networking bugs.

If you or others think that a larger scope that either solves this on a per request and/or cross platform basis is preferable I am all for it, I'm just not sure how helpful I would be with the native code changes.

Please let me know how I can help with this and thank you for your assistance!

[ericlewis]

@bondparkerbond thank you for the extremely thoughtful response, and it has now added new questions to my mind! I will say that making it global and on by default is probably not the best idea in iOS land, where the system does its best to try and manage its networking capabilities (such as switching from wifi when the range isn't so great) there is granularity in iOS to change some of these behaviors (such as switching off cellular data for individual apps) without needing to create a default. Obviously its best if we have some support for this, but I feel that as we are trying to "cut scope" in some ways, the justification for new features needs to rise to new levels, and include android as part of it. I think it is important to core, but I also think we are beyond a simple change, and would love feedback from @shergin @TheSavior @cpojer on their feelings about this. And maybe they can point us towards help on the android side! :D

[bondparkerbond]

@ericlewis I agree that we do not want to change the default behavior for iOS(or Android) RN apps.

However, I do think it would be valuable to allow a little more granular control over api calls and networking in general when needed. Ideally, RN devs could access most/all of the functionality that native devs have out of the box in a RN App without needing to reimplement their own networking module from scratch.

I think that one way we could accomplish forcing WiFi for API calls in particular would be by allowing access to set the property allowsCellularAccess on iOS and to set the TransportType on Android to NetworkCapabilities.TRANSPORT_WIFI.

This would allow us to keep feature parity across platforms and allow an escape hatch for those of us who require WiFi specifically and not cellular data. I would imagine this being particularly useful when communicating with a local IoT device/wifi router/etc. as well as for other reasons such as data caps on 4G, etc.

Do you think the above idea could work or might we want to handle it in a different way? Regardless of the implementation, we'd also need to answer the following question: how/where do we allow configuring this in an app to override the current default behavior?

I'd love to hear any thoughts you, @cpojer, @shergin, or @TheSavior might have on how we might best go about this. Thank you for taking the time to look into this and have a good day!

[ericlewis]

@bondparkerbond have you tried setting UIRequiresPersistentWiFi in the app's info PLIST file? This will indicate to iOS that your app requires it be connected to wifi. If that still doesn't satisfy the requirement, we could consider adding our own flag to the info PLIST.

[bondparkerbond]

@ericlewis I already have UIRequiresPersistentWiFi set to true in my Info.plist. However, as far as I understand, it does something different than the allowsCellularAccess property.

Having UIRequiresPersistentWiFi: true will prevent the apps WiFi connection from being shut off after 30 minutes, and it "lets the system know that it should display the network selection dialog if it detects any active Wi-Fi hot spots".source

However, it does not affect whether or not 4G is also used in the App, hence the need for the allowsCellularAccess property, which will fail network calls if only 4G is available, and will cause the app to behave as if no cellular data connection were present, which is the desired behavior in the use case of needing to communicate only with a local hardware device over WiFi.

I think that using a custom plist flag for overriding the default value of "allowsCellularAccess: true" would be a great way to expose the functionality that UIRequiresPersistentWiFi does not have and would expose the desired functionality regarding only communicating over WiFi in the App. While it is not as granular as allowingCellularAccess on a per call basis, it should be much more strait-forward to implement and maintain, and I would view it as a perfectly reasonable tradeoff for apps that need such functionality.

Please let me know if this is the way the RN team wishes to go and if so I will start work on documentation, etc. for this PR.

[cpojer]

I think this should be a build time config/flag that is by default off. @ericlewis was proposing adding a new config in a plist file to enable this feature specifically. If you can change your solution to something like this, without changing the existing behavior, we can merge this.

[bondparkerbond]

I'll see about getting something together for next week. Thanks for the feedback!

[bondparkerbond]

Does anyone have any preference on the Key name to add to the Info.plist to override default behavior on iOS? I'm thinking of something like RCTForceAppleWifiOnly but if there is already a standard I can use something else.

[zhigang1992]

IMHO, it's should be something close to allowsCellularAccess, like disallowsCellularAccess? This way can have an easier time to figure out what it is later on.

[bondparkerbond]

@cpojer I have added a commit to allow overriding the default behavior of allowsCellularAccess if users provide a custom key inside Info.plist.

I think this should satisfy the requirements you mentioned although I'm open to any changes or improvements if you or @ericlewis have any feedback on naming or if we want to remove some of the comments I added in favor of adding something to the CHANGELOG, etc.

If you don't have any problems with it as implemented we can mark this Draft PR as Ready for review.

[cpojer]

Yeah this looks good to me. Can you publish it?

cc @ericlewis mind taking a look before we merge this?

[ericlewis]

Functionally okay, lets try to simplify it even further.

[bondparkerbond]

@ericlewis To clarify, are you referring to changing the key in Info.plist from ReactNetworkConfigChoice to ReactNativeNetworkWifiOnly, looking to rename customOverrideKey to ReactNativeNetworkWifiOnly, or both/something else?

I think it could be merged in now as is, so I'm going to remove the draft status from this PR, but if you want to change the naming of things we can still do that, I'm just not clear what you want renamed.

Also, it would be good if we kept the names from getting longer than they currently are otherwise we might go over 110 characters on some lines, and I'm not sure what the lint rules are for line length in this repo.

[bondparkerbond]

@ericlewis I made some changes as requested and think it should be ready to merge in if you're happy with it. cc @cpojer As I don't have merge access.

[ericlewis]

LGTM!

[cpojer]

Thank you!

[facebook-github-bot]

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[react-native-bot]

This pull request was successfully merged by @bondparkerbond in 01c70f2.

^{When will my fix make it into a release? | Upcoming Releases}

[zhongwuzw]

@cpojer Can we change this to Bool? we just have two values YES or NO, don't need to do string comparison.

[cpojer]

I don't have strong feelings on it. @ericlewis @bondparkerbond what do you think? If you think we should change this, please send a follow-up PR.

[zhongwuzw]

@ericlewis @bondparkerbond Here we go, please see #24829.