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

[iOS] Change ReactNetworkForceWifiOnly from String to Boolean #24829

Conversation

zhongwuzw
Copy link
Contributor

@zhongwuzw zhongwuzw commented May 13, 2019

Summary

We provided ReactNetworkForceWifiOnly in #24242, but it's a string type, actually the value only YES or NO, so let's change it to Boolean type, we can benefit from:

  1. Users don't need to type string YES, just select bool YES or NO directly by click the button:

image

  1. Don't need to do string compare.

  2. More looks what it should looks, Boolean is the Boolean. :)

cc. @cpojer @ericlewis .

Changelog

[iOS] [Changed] - Change ReactNetworkForceWifiOnly from String to Boolean

Test Plan

CI passes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label May 13, 2019
@cpojer cpojer requested a review from ericlewis May 13, 2019 15:42
@cpojer
Copy link
Contributor

cpojer commented May 13, 2019

cc @bondparkerbond

@react-native-bot react-native-bot added the Platform: iOS iOS applications. label May 13, 2019
@ericlewis
Copy link
Contributor

if @zhongwuzw says it works it probably works ;)

@bondparkerbond
Copy link
Contributor

I initially tried that but kept getting errors. (Perhaps because I was trying to manually add it into the Info.plist file instead of creating it inside Xcode?) I saw references to user defined Bool values not being allowed in this Stack Overflow Response.
Specifically, the 3rd answer claimed "plist files containing booleans within tags are not valid any more", but I could find no other documentation specifically saying it was/wasn't allowed. Since I am relatively new to Obj C I decided to go with the way that I had tested and knew would work, but if @zhongwuzw was able to test it and it worked I see no reason not to merge this in. We may want to specify in the file to make sure to set this in Xcode before merging though.

@bondparkerbond
Copy link
Contributor

bondparkerbond commented May 13, 2019

One thing I do want to double check because I'm not sure on the expected behavior: since bool values are apparently just 0/1, but are shown as YES/NO in Xcode and <true/>/<false/> when you examine the actual Info.plist values, are the following equivalent?:

configuration.allowsCellularAccess = !1 (or !YES)
configuration.allowsCellularAccess = 0
configuration.allowsCellularAccess = NO;

Maybe it would be better to do something like:

if(plist value exists AND is true/YES/1?) {
  configuration.allowsCellularAccess = NO;
}

(ie: literally writing NO like we are writing YES for setHTTPShouldSetCookies:YES on the next line)
This would be more explicit that we are setting a value to NO as we don't need to set = YES as that is the default if nothing is set.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Works for me.

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@zhongwuzw
Copy link
Contributor Author

One thing I do want to double check because I'm not sure on the expected behavior: since bool values are apparently just 0/1, but are shown as YES/NO in Xcode and <true/>/<false/> when you examine the actual Info.plist values, are the following equivalent?:

configuration.allowsCellularAccess = !1 (or !YES)
configuration.allowsCellularAccess = 0
configuration.allowsCellularAccess = NO;

Maybe it would be better to do something like:

if(plist value exists AND is true/YES/1?) {
  configuration.allowsCellularAccess = NO;
}

The three cases are the same. YES/NO, true/false, 1/0 have no practical difference.

(ie: literally writing NO like we are writing YES for setHTTPShouldSetCookies:YES on the next line)
This would be more explicit that we are setting a value to NO as we don't need to set = YES as that is the default if nothing is set.

Emm, actually, based on standard OC coding style, we don't write if (setHTTPShouldSetCookies == YES), we should write if (setHTTPShouldSetCookies).

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @zhongwuzw in 916186a.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 14, 2019
facebook-github-bot pushed a commit that referenced this pull request Sep 6, 2022
Summary:
This sync includes the following changes:
- **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([#24846](facebook/react#24846)) //<Andrew Clark>//
- **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([#24830](facebook/react#24830)) //<Luna Ruan>//
- **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766))" ([#24829](facebook/react#24829)) //<Andrew Clark>//
- **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766)) //<Luna Ruan>//
- **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([#24585](facebook/react#24585)) //<Andrew Clark>//
- **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([#24749](facebook/react#24749)) //<Andrew Clark>//
- **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([#24817](facebook/react#24817)) //<Andrew Clark>//
- **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([#24806](facebook/react#24806)) //<Luna Ruan>//
- **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([#24801](facebook/react#24801)) //<Luna Ruan>//
- **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([#24686](facebook/react#24686)) //<Luna Ruan>//
- **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([#24784](facebook/react#24784)) //<Josh Story>//
- **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([#24776](facebook/react#24776)) //<Luna Ruan>//
- **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([#24765](facebook/react#24765)) //<Luna Ruan>//
- **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([#24754](facebook/react#24754)) //<Sebastian Markbåge>//
- **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([#24752](facebook/react#24752)) //<Sebastian Markbåge>//
- **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([#24753](facebook/react#24753)) //<Sebastian Markbåge>//
- **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([#24751](facebook/react#24751)) //<Sebastian Markbåge>//
- **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([#24732](facebook/react#24732)) //<Luna Ruan>//
- **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([#24742](facebook/react#24742)) //<Mengdi Chen>//
- **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([#24612](facebook/react#24612)) //<Kerim Büyükakyüz>//

Changelog:
[General][Changed] - React Native sync for revisions 229c86a...c1f5884

Reviewed By: mdvacca, GijsWeterings

Differential Revision: D38904311

fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
cipolleschi pushed a commit that referenced this pull request Sep 7, 2022
Summary:
This sync includes the following changes:
- **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([#24846](facebook/react#24846)) //<Andrew Clark>//
- **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([#24830](facebook/react#24830)) //<Luna Ruan>//
- **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766))" ([#24829](facebook/react#24829)) //<Andrew Clark>//
- **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([#24766](facebook/react#24766)) //<Luna Ruan>//
- **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([#24585](facebook/react#24585)) //<Andrew Clark>//
- **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([#24749](facebook/react#24749)) //<Andrew Clark>//
- **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([#24817](facebook/react#24817)) //<Andrew Clark>//
- **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([#24806](facebook/react#24806)) //<Luna Ruan>//
- **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([#24801](facebook/react#24801)) //<Luna Ruan>//
- **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([#24686](facebook/react#24686)) //<Luna Ruan>//
- **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([#24784](facebook/react#24784)) //<Josh Story>//
- **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([#24776](facebook/react#24776)) //<Luna Ruan>//
- **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([#24765](facebook/react#24765)) //<Luna Ruan>//
- **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([#24754](facebook/react#24754)) //<Sebastian Markbåge>//
- **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([#24752](facebook/react#24752)) //<Sebastian Markbåge>//
- **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([#24753](facebook/react#24753)) //<Sebastian Markbåge>//
- **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([#24751](facebook/react#24751)) //<Sebastian Markbåge>//
- **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([#24732](facebook/react#24732)) //<Luna Ruan>//
- **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([#24742](facebook/react#24742)) //<Mengdi Chen>//
- **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([#24612](facebook/react#24612)) //<Kerim Büyükakyüz>//

Changelog:
[General][Changed] - React Native sync for revisions 229c86a...c1f5884

Reviewed By: mdvacca, GijsWeterings

Differential Revision: D38904311

fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[c1f5884ff](facebook/react@c1f5884ff )**: Add missing null checks to OffscreenInstance code ([facebook#24846](facebook/react#24846)) //<Andrew Clark>//
- **[4cd788aef](facebook/react@4cd788aef )**: Revert "Revert [Transition Tracing] Refactor Transition Tracing Root Code" ([facebook#24830](facebook/react#24830)) //<Luna Ruan>//
- **[e61fd91f5](facebook/react@e61fd91f5 )**: Revert "[Transition Tracing] Refactor Transition Tracing Root Code ([facebook#24766](facebook/react#24766))" ([facebook#24829](facebook/react#24829)) //<Andrew Clark>//
- **[401296310](facebook/react@401296310 )**: [Transition Tracing] Refactor Transition Tracing Root Code ([facebook#24766](facebook/react#24766)) //<Luna Ruan>//
- **[185932902](facebook/react@185932902 )**: Track nearest Suspense handler on stack ([facebook#24585](facebook/react#24585)) //<Andrew Clark>//
- **[a7b192e0f](facebook/react@a7b192e0f )**: Add test gate alias for Offscreen ([facebook#24749](facebook/react#24749)) //<Andrew Clark>//
- **[6b6cf8311](facebook/react@6b6cf8311 )**: Land forked reconciler changes ([facebook#24817](facebook/react#24817)) //<Andrew Clark>//
- **[d1432ba93](facebook/react@d1432ba93 )**: [Transition Tracing] Fix excess calls to the transition start callback ([facebook#24806](facebook/react#24806)) //<Luna Ruan>//
- **[88574c1b8](facebook/react@88574c1b8 )**: Fix enableTransitionTracing flag ([facebook#24801](facebook/react#24801)) //<Luna Ruan>//
- **[a4bed4696](facebook/react@a4bed4696 )**: [Transition Tracing] Add Tracing Markers ([facebook#24686](facebook/react#24686)) //<Luna Ruan>//
- **[167853026](facebook/react@167853026 )**: fix hydration warning suppression in text comparisons ([facebook#24784](facebook/react#24784)) //<Josh Story>//
- **[9abe745aa](facebook/react@9abe745aa )**: [DevTools][Timeline Profiler] Component Stacks Backend ([facebook#24776](facebook/react#24776)) //<Luna Ruan>//
- **[cf665c4b7](facebook/react@cf665c4b7 )**: [DevTools] Refactor incompleteTransitions field from Root Fiber memoized state to FiberRoot ([facebook#24765](facebook/react#24765)) //<Luna Ruan>//
- **[56389e81f](facebook/react@56389e81f )**: Abort Flight ([facebook#24754](facebook/react#24754)) //<Sebastian Markbåge>//
- **[0f216ae31](facebook/react@0f216ae31 )**: Add entry points for "static" server rendering passes ([facebook#24752](facebook/react#24752)) //<Sebastian Markbåge>//
- **[f796fa13a](facebook/react@f796fa13a )**: Rename Segment to Task in Flight ([facebook#24753](facebook/react#24753)) //<Sebastian Markbåge>//
- **[0f0aca3ab](facebook/react@0f0aca3ab )**: Aborting early should not infinitely suspend ([facebook#24751](facebook/react#24751)) //<Sebastian Markbåge>//
- **[12a738f1a](facebook/react@12a738f1a )**: [Transition Tracing] Add Support for Multiple Transitions on Root ([facebook#24732](facebook/react#24732)) //<Luna Ruan>//
- **[72ebc703a](facebook/react@72ebc703a )**: [DevTools] fix useDeferredValue to match reconciler change ([facebook#24742](facebook/react#24742)) //<Mengdi Chen>//
- **[7cf9f5e03](facebook/react@7cf9f5e03 )**: Extra space ([facebook#24612](facebook/react#24612)) //<Kerim Büyükakyüz>//

Changelog:
[General][Changed] - React Native sync for revisions 229c86a...c1f5884

Reviewed By: mdvacca, GijsWeterings

Differential Revision: D38904311

fbshipit-source-id: 1e30bc420c30ec7a0c0073fc92a706afef4b3340
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants