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

Implemented automatic IP detection for iOS #8091

Closed
wants to merge 6 commits into from

Conversation

nathanajah
Copy link
Contributor

Implemented automatic IP detection for iOS, based on #6345 and #6362.
As the previous pull requests did, this works by writing the IP address of the host to a file.

Test plan:
npm pack on the commit and create a new project by doing react-native init using the generated package.
With both the computer and the device on the same wireless connection, run the generated project on the device.
Make sure that the device loads the bundle successfully from the packager.
Turn off the packager, and run the app again.
Make sure that the device loads the pre-packaged bundle successfully.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @geof90 and @frantic to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jun 13, 2016
{
NSString *ipPath = [[NSBundle mainBundle] pathForResource:@"ip" ofType:@"txt"];
NSString *ip = [NSString stringWithContentsOfFile:ipPath encoding:NSUTF8StringEncoding error:nil];
ip = [ip stringByTrimmingCharactersInSet:[NSCharacterSet characterSetWithCharactersInString:@"\n"]];
Copy link
Member

Choose a reason for hiding this comment

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

Use [NSCharacterSet newlineCharacterSet]. I'd also assign it immediately to ipGuess instead of making that a separate assignment.

@javache
Copy link
Member

javache commented Jun 13, 2016

Looks awesome!


@implementation RCTBundleURLProvider

+ (void)initialize
Copy link
Member

Choose a reason for hiding this comment

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

Can you gate this with RCT_DEV? We don't want to run this in production.


@implementation RCTBundleURLProvider

+ (void)initialize
{
#if RCT_DEV
Copy link
Member

Choose a reason for hiding this comment

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

You should put this outside the + (void)initialize, otherwise we'll still be executing an empty method in prod.

@javache
Copy link
Member

javache commented Jun 13, 2016

@facebook-github-bot shipit

@ghost ghost added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jun 13, 2016
@ghost
Copy link

ghost commented Jun 13, 2016

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ghost ghost closed this in 8c29a52 Jun 13, 2016
MattFoley pushed a commit to skillz/react-native that referenced this pull request Jun 17, 2016
Summary:
Implemented automatic IP detection for iOS, based on facebook#6345 and facebook#6362.
As the previous pull requests did, this works by writing the IP address of the host to a file.
Closes facebook#8091

Differential Revision: D3427657

Pulled By: javache

fbshipit-source-id: 3f534c9b32c4d6fb9615fc2e2c3c3aef421454c5
@jsierles
Copy link
Contributor

Nice work on this! Just wanted to comment on a couple things. It looks like the default settings are never explicitly enabled on boot, so dev mode is disabled by default now, unless you add this to AppDelegate:

[[RCTBundleURLProvider sharedSettings] setEnableDev:TRUE];

It looks like the intent was to set defaults at initialization, but setDefaults never gets called.

Also noticed a typo here: https://github.com/facebook/react-native/blob/master/React/Base/RCTBundleURLProvider.m#L165

Happy to submit a PR once I understand the original intention on setting defaults!

@javache
Copy link
Member

javache commented Jun 22, 2016

Looks like we're missing a call to setDefaults yeah. @nathanajah: can you add this?

@jondot
Copy link
Contributor

jondot commented Jun 22, 2016

Turning your attention to this, already done via https://github.com/jondot/react-native-network-boot where I believe my approach unified edge cases such as multiple interfaces, virtualbox, etc. and doesn't touch React Native core.

@jsierles
Copy link
Contributor

Added #8400 to resolve this. @jondot, your work looks good, maybe submit a PR with the improved IP detection? I think it's important for this to live in React Native core, as testing on an iOS device today is a frustrating experience for first-time users.

@nathanajah
Copy link
Contributor Author

Hi @jsierles! Sorry for forgetting to reply here, but commit 9845f49 should have resolved it. Thanks!

@jsierles
Copy link
Contributor

Thanks! However shouldn't the defaults be set without having to call that method? Since they are defaults.

@jsierles
Copy link
Contributor

@nathanajah Added a new PR to only address the defaults not being set automatically: #8404

@nihgwu
Copy link
Contributor

nihgwu commented Jun 26, 2016

I think it should be addressed about the development mode issue in the release note of 0.29RC or release a new RC version with the patch shipped, since i've upgrade my app to 0.29 and suffered a lot from this issue
I changed AppDelegate.m to the new form and got various errors such as

ExceptionsManager.js:55Module HMRClient is not a registered callable module.

or

ExceptionsManager.js:55locals[0] does not appear to be a `module` object with Hot Module replacement API enabled. You should disable react-transform-hmr in production by using `env` section in Babel configuration. See the example in README: https://github.com/gaearon/react-transform-hmr

if I enabled the hot reload module feature

It's really annoying and frustrating as I can't run my app then, I tried everything I can do such as react-native upgrade or reinstall the node_modules or clear and rebuild the project and finally I noticed that in the terminal it was requesting the bundle with dev=false
I've solved the problem by launch the commit 9845f49 manually now

@jondot
Copy link
Contributor

jondot commented Jul 4, 2016

@jsierles just to verify, you think it's important to live in react core (i've been using this in my apps as an npm module)? and then, do you think i should submit a new PR for this, together with a modification for the default template that uses it?

Thanks!

samerce pushed a commit to iodine/react-native that referenced this pull request Aug 23, 2016
Summary:
Implemented automatic IP detection for iOS, based on facebook#6345 and facebook#6362.
As the previous pull requests did, this works by writing the IP address of the host to a file.
Closes facebook#8091

Differential Revision: D3427657

Pulled By: javache

fbshipit-source-id: 3f534c9b32c4d6fb9615fc2e2c3c3aef421454c5
mpretty-cyro pushed a commit to HomePass/react-native that referenced this pull request Aug 25, 2016
Summary:
Implemented automatic IP detection for iOS, based on facebook#6345 and facebook#6362.
As the previous pull requests did, this works by writing the IP address of the host to a file.
Closes facebook#8091

Differential Revision: D3427657

Pulled By: javache

fbshipit-source-id: 3f534c9b32c4d6fb9615fc2e2c3c3aef421454c5
cpojer pushed a commit to facebook/metro that referenced this pull request Jan 26, 2017
Summary:
Implemented automatic IP detection for iOS, based on #6345 and #6362.
As the previous pull requests did, this works by writing the IP address of the host to a file.
Closes facebook/react-native#8091

Differential Revision: D3427657

Pulled By: javache

fbshipit-source-id: 3f534c9b32c4d6fb9615fc2e2c3c3aef421454c5
facebook-github-bot pushed a commit that referenced this pull request Jan 26, 2018
Summary:
By default, when a react-native app launches in development mode on a physical iOS device, it attempts to load the JS bundle from a packager at `http://_your-local-ip-address_.xip.io:8081/`.

This change removes the use of `xip.io`, which changes that url to: `http://_your-local-ip-address_:8081/`

<!--
Thank you for sending the PR! We appreciate you spending the time to work on these changes.

Help us understand your motivation by explaining why you decided to make this change.

You can learn more about contributing to React Native here: http://facebook.github.io/react-native/docs/contributing.html

Happy contributing!

-->

Background:
The automatic IP detection feature (introduced [here](#8091)) is super handy. However, it’s use of `xip.io` has caused myself and others much grief.  Some routers do not allow or have intermittent errors when trying to resolve DNS names to local IP addresses. This prompted the introduction of a [DISABLE_XIP feature](#13326), which helps.

However, I don’t believe the use of `xip.io` is needed at all.

Based on [this comment](8c29a52#commitcomment-18224788), it appears the original reason for using `xip.io` was to “circumvent the numeric IP address limitation in ATS”.

But, the reason you can’t create ATS exceptions for raw IP addresses is that ATS is not enforced for raw IP addresses _at all_. You can read the Apple documentation [here](https://developer.apple.com/library/content/documentation/General/Reference/InfoPlistKeyReference/Articles/CocoaKeys.html), the relevant portion is:

> App Transport Security (ATS) applies only to connections made to public host names. The system does not provide ATS protection to connections made to:
> * Internet protocol (IP) addresses
> * Unqualified host names
> * Local hosts employing the .local top-level domain (TLD)

For example, in iOS, if you attempt to make an http request (note: _not_ https) to `http://www.google.com` you will get an error due to ATS.
However, you can make the same request to `http://172.217.6.14/`  (which for me is the same server) and the request will succeed.

And in fact, if an ATS exception _was_ needed, the DISABLE_XIP feature shouldn’t work at all, but it does.

In short, using `xip.io` with ATS exceptions is unnecessary, causes some very annoying problems for some people, and I think it should just be removed.

Run the app on a physical iOS device and verify that it can load the JS bundle from the host computer's IP.

[Implemented automatic IP detection for iOS #8091](#8091)
[Added option to disable xip #13326](#13326)

<!--
Help reviewers and the release process by writing your own release notes

**INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

  CATEGORY
[----------]        TYPE
[ CLI      ]   [-------------]      LOCATION
[ DOCS     ]   [ BREAKING    ]   [-------------]
[ GENERAL  ]   [ BUGFIX      ]   [-{Component}-]
[ INTERNAL ]   [ ENHANCEMENT ]   [ {File}      ]
[ IOS      ]   [ FEATURE     ]   [ {Directory} ]   |-----------|
[ ANDROID  ]   [ MINOR       ]   [ {Framework} ] - | {Message} |
[----------]   [-------------]   [-------------]   |-----------|

[CATEGORY] [TYPE] [LOCATION] - MESSAGE

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
 [INTERNAL] [BUGFIX] [./scripts] - Removed use of xip.io
Closes #17642

Differential Revision: D6814609

Pulled By: hramos

fbshipit-source-id: f2faebd6a29b0b211d78cdfe8e195906307ab552
grabbou pushed a commit to react-native-community/cli that referenced this pull request Sep 26, 2018
Summary:
Implemented automatic IP detection for iOS, based on #6345 and #6362.
As the previous pull requests did, this works by writing the IP address of the host to a file.
Closes facebook/react-native#8091

Differential Revision: D3427657

Pulled By: javache

fbshipit-source-id: 3f534c9b32c4d6fb9615fc2e2c3c3aef421454c5
This pull request was closed.
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. Import Started This pull request has been imported. This does not imply the PR has been approved.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants