Skip to content

Conversation

@aleclarson
Copy link
Contributor

@aleclarson aleclarson commented Mar 8, 2016

Fixes #6291, #4351, #4245, #1597, and #883.

It's worth mentioning that I did not change the call to /status because it's in a "Run Script" phase. Any ideas on fixing that as well?

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @foghina, @nicklockwood and @javache 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 Mar 8, 2016
@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

@javache
Copy link
Member

javache commented Mar 8, 2016

I'm not a huge fan of doing this automatically, as at some point, it will be very confusing where this IP is coming from, the wrong IP will be used, or people won't find the magic shell scripts embedded in the xcode project.

@Kureev
Copy link
Contributor

Kureev commented Mar 8, 2016

@aleclarson looks nice 👍

@aleclarson
Copy link
Contributor Author

@javache

  1. Is it worth moving the shell script to a .sh file for easier finding?
  2. Maybe we could have a manually set IP that overrides the automatically set IP?
  3. Necessary fixes to the shell script will appear in the issues over time. 😄

@Kureev
Copy link
Contributor

Kureev commented Mar 8, 2016

@aleclarson

Maybe we could have a manually set IP that overrides the automatically set IP?

I think it should be something like an optional parameter: react-native run-ios --host=192.168.1.10

@aleclarson
Copy link
Contributor Author

@Kureev Ooh I really like that.

We could also take advantage of NSUserDefaults just like the port uses [standardDefaults integerForKey:@"websocket-executor-port"].

@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

@aleclarson
Copy link
Contributor Author

The recent changes include:

  • The script that starts the React Packager now uses the automatic IP address.
  • Combined the IP script and Packager script into local-cli/start_packager.sh.

Changes still needed:

  • Add --host and --port flags to the run-ios command.
  • Somehow access the custom host and port inside the start_packager.sh script.

echo "// Updated on "`date` >> $HEADER
echo "" >> $HEADER

ip_address="`ifconfig en0 | grep "inet " | cut -d: -f2 | awk '{print $2}'`"
Copy link
Contributor

Choose a reason for hiding this comment

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

This depends strongly on your system (only macbooks on wifi, right?). And if you assume that you're on a mac, you could just use ipconfig getifaddr en0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mroswald I just copied that line from a commit linked by this comment. This has also been discussed in depth in #4245. It can't be ipconfig getifaddr en0 because you might change networks often (eg: you go to a coffee shop, or you use ethernet at work). I doubt what I have is the perfect solution; but as long as there is a way to override the automatic IP, we can use the current solution. If anyone finds a way to make it more robust, just submit a PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I still don't fully understand what happens when you have, say, en0 and en1? Does it pick the random one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I haven't tested that scenario. Maybe @christopherdro knows.

Copy link
Contributor

Choose a reason for hiding this comment

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

@frantic has a good point that this might not work for all cases.

I believe new macs that are only wireless use en0 however, older macs that use en0 when wired in and en1 for wifi.

Copy link
Contributor

Choose a reason for hiding this comment

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

ipconfig getifaddr en0 is less magical and will be easier to fix in the future. If we want to go the more complex route, we can use ifconfig command to find and store all available interfaces, and somehow offer the user to choose from the list. I don't think it's worth it at this point.

@frantic
Copy link
Contributor

frantic commented Mar 10, 2016

Slightly different approach for addressing the same problem: #6345

I guess the main difference is that in #6345 we store the IP address in a file inside the main app's bundle, but here we modify a header file. I guess both have pro's and con's. In Facebook's case we have a copy of RN inside the repo, which means every Xcode build will change the header with the IP and might cause troubles. Using a text file is nice when we consider a potential use case of downloading pre-built binaries (instead of compiling the shell ourselves), in which case it's much easier to swap the IP.

I like that RCTServerUtils also abstracts away the port as well (if we decide to change it at some point).

@javache good point, but I think it's worth it. Now basically everyone has to mess with the IP setting every time they want to test on device. We can solve the 80%-case and have clearer error messages on how to deal with the rest 20%.

@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

2 similar comments
@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

@aleclarson
Copy link
Contributor Author

Weird. I haven't updated the pull request in 6 days, but the bot keeps saying I'm updating it. 😯

@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

@ghost
Copy link

ghost commented Apr 17, 2016

@foghina would you mind taking a look at this pull request? It's been a while since the last commit was reviewed.

@christopherdro
Copy link
Contributor

Sorry for the late response.

This is what I've been using internally for the last couple months and has been serving us well.

wildlifela@a98fc4d

Then update your AppDelegate.m.

#import "RCTIPAddress.h"

 #if RCT_DEV
      #if TARGET_OS_SIMULATOR
        jsCodeLocation = [NSURL URLWithString:@"http://localhost:8081/index.ios.bundle?platform=ios"];
      #else
        NSString *URLString = [NSString stringWithFormat:@"http://%@:8081/index.ios.bundle?platform=ios", LOCAL_IP_ADDRESS ];
        jsCodeLocation = [NSURL URLWithString:URLString];
      #endif
    #else
      jsCodeLocation = [[NSBundle mainBundle] URLForResource:@"main" withExtension:@"jsbundle"];
    #endif

@rauchy
Copy link

rauchy commented May 17, 2016

@ghost
Copy link

ghost commented May 28, 2016

@aleclarson do you have any updates for this pull request? It's been a while since the last update so wanted to check in and see if you've looked at the requested changes.

@ghost
Copy link

ghost commented May 28, 2016

@aleclarson updated the pull request.

@aleclarson
Copy link
Contributor Author

I noticed RCTDevMenu has a workaround of its own:

- (NSURL *)packagerURL
{
  NSString *host = [_bridge.bundleURL host];
  if (!host) {
    return nil;
  }

  NSString *scheme = [_bridge.bundleURL scheme];
  NSNumber *port = [_bridge.bundleURL port];
  if (!port) {
    port = @8081; // Packager default port
  }
  return [NSURL URLWithString:[NSString stringWithFormat:@"%@://%@:%@/message?role=shell", scheme, host, port]];
}

If RCTPackagerUtils had access to the RCTBridge, we could just go with that solution.

Otherwise, we can just replace that code with [RCTPackagerUtils URLForPath:].

@facebook-github-bot
Copy link
Contributor

@aleclarson updated the pull request.

@javache
Copy link
Member

javache commented May 28, 2016

Hi @aleclarson, @nathanajah is working on open-sourcing our internal component that provides automatic packager discovery and will incorporate your changes. Thank you for your work on this PR!

@aleclarson
Copy link
Contributor Author

@javache Sounds good!

@aleclarson aleclarson closed this May 28, 2016
ghost pushed a commit that referenced this pull request Jun 13, 2016
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 #8091

Differential Revision: D3427657

Pulled By: javache

fbshipit-source-id: 3f534c9b32c4d6fb9615fc2e2c3c3aef421454c5
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
@aleclarson aleclarson deleted the localIP-PR branch June 19, 2016 09:24
bubblesunyum 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
@isaaclem
Copy link
Contributor

Is there any recommended way to handle on running ios devices?

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Chrome debugging from phone

10 participants