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

[Packager] Add option for remote packager in info.plist #548

Closed
wants to merge 1 commit into from
Closed

[Packager] Add option for remote packager in info.plist #548

wants to merge 1 commit into from

Conversation

ghost
Copy link

@ghost ghost commented Mar 31, 2015

Add an option to add a ReactServer key to info.plist that specifies a location to a remote server running the packager. This allows you to easily change the location of the packager for on-device debugging without having to root around in various parts of the code, namely AppDelegate.m and RCTWebSocketExecutor.m (if you want debugging).

@amasad
Copy link
Contributor

amasad commented Apr 1, 2015

@frantic can you take a look?

@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 Apr 7, 2015
@brentvatne
Copy link
Collaborator

  • Should follow styleguide - curly braces for all if statements, space between if and paren
  • Any change here would need to include a corresponding change to the docs in docs/Debugging.md and perhaps other places

cc @tobyhughes @frantic

@frantic
Copy link
Contributor

frantic commented May 14, 2015

@tobyhughes - if you are still interested in working on that, please consider @brentvatne's suggestions about code style and docs. Potentially (in separate PR) we could also provide a script that at build time writes laptop's IP address into Info.plist to make everything "just work".

@ghost
Copy link
Author

ghost commented May 14, 2015

Will do :) I'll update the PR tonight.

@frantic
Copy link
Contributor

frantic commented May 15, 2015

Just looked at the codebase, and seems like there are few more places where the URL is hardcoded. Would be nice to change those refs too:

  • Libraries/WebSocket/RCTWebSocketExecutor.m
  • React/Base/RCTRedBox.m
  • Libraries/RCTTest/RCTTestRunner.m

You can also extract this logic into some util function to not have to do this manually in different places, something like RCTServerURLWithPath(@"/index.bundle")

@ide
Copy link
Contributor

ide commented May 15, 2015

Could you generalize this so it's programmatically configurable and reads from the Info.plist by default but doesn't require it? Ex: [RCTConfiguration sharedConfiguration][@"scriptHost"] = ipAddress.

@sahrens
Copy link
Contributor

sahrens commented May 16, 2015

cc @nicklockwood - were you planning on adding this to the dev menu? would be nice to move what we have internally to OSS with the bonjour stuff and app link support (rn://ip:X.X.X.X)

@jtremback
Copy link
Contributor

Is there any way I can help with this? I've got to debug on device because my app uses the camera, and this stuff is a pain.

{
NSString *url = [[NSBundle mainBundle] objectForInfoDictionaryKey:@"ReactServer"];

if (url == nil){
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: space after )

@frantic
Copy link
Contributor

frantic commented May 29, 2015

I'm getting this in shortly :)

@nicklockwood
Copy link
Contributor

Yeah, this isn't the right right way to handle URLs in iOS. You should probably do something like:

NSString *server = [[NSBundle mainBundle] objectForInfoDictionaryKey:@"ReactServer"];
if (server == nil) {
    server = @"http://localhost:8081";
}
NSURL *baseURL = [NSURL URLWithString:server];
NSURL *fullURL = [NSURL URLWithString:bundlePath relativeToURL:baseURL];

Although that also isn't completely bulletproof. Probably worth writing some unit tests with various combinations of missing or redundant prefixes and suffixes.

It may be worth using [RCTConvert NSURL:server] instead of [NSURL URLWithString:server] actually, as that handles a whole bunch of bad inputs gracefully already, and already has some tests.

@frantic
Copy link
Contributor

frantic commented Jun 1, 2015

@nicklockwood has a great point

@brentvatne brentvatne changed the title Add option for remote packager in info.plist [Packager] Add option for remote packager in info.plist Jun 1, 2015
@brentvatne
Copy link
Collaborator

@tobyhughes - any interest in updating this or can we close?

@ghost ghost closed this Jun 1, 2015
acoates-ms added a commit to acoates-ms/react-native that referenced this pull request Aug 18, 2020
* Align some more of the publish/pr logic on android builds

* minor fix

Co-authored-by: Andrew Coates <acoates-ms@noreply.github.com>
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants