Support for configurable debug origin/port (issue 1429) #1546

bparadie · 2015-06-07T20:33:05Z

This PR adds support for configurable debug origin/port information as discussed in #1429 .

Summary

Here is a brief summary of my changes:

I added a new target called ReactWithoutPackager to React.xcodeproj that is a copy of the React target but does not execute the launchPackager.command script.
UIExplorer now links against libReactWithoutPackager.a, which attracts the ReactWithoutPackager target w/o launchPackager.command.
I added support for --origin parameter to packager.sh
I added a run script to UIExplorer that launches launchPackager.command with --origin and --port parameters.
Because I am now passing --origin and --port as parameters to launchPackager.command and open w/t --args does not seem to work with Terminal, I changed that code to using osascript instead.

Changing the port number

This is a screenshot after changing the port number to 8086 in Info.plist and rebuilding UIExplorer:

Backwards compatibility

I decided to make this change backwards compatible so clients can choose to continue using the old hard-coded origin/port (http://localhost:8081). The main reason is that many devs probably pick an example project and start modifying it to fit their needs. If we required Info.plist entries for origin/port values those old modified examples would break.

Usage in examples

At this point I only modified UIExplorer to show that origin/port can be configured. But I could change all examples if needed.

Utility function

There are three places where I added code that looks up the values for origin/port from Info.plist. That code is fairly trivial but could still be extracted and shared as a utility function. Please advise if that is necessary and let me know where I should add that utility function to.

facebook-github-bot · 2015-06-07T20:36:47Z

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

facebook-github-bot · 2015-06-07T23:02:43Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

amasad · 2015-06-15T21:21:21Z

I believe @frantic is the best person to review this

frantic · 2015-06-15T23:01:17Z

Examples/UIExplorer/UIExplorer.xcodeproj/project.pbxproj

+			);
+			runOnlyForDeploymentPostprocessing = 0;
+			shellPath = /bin/bash;
+			shellScript = "export ORIGIN=$(/usr/libexec/PlistBuddy -c \"Print :RCTWebSocketExecutor:origin\" \"${INFOPLIST_FILE}\")\nexport PORT=$(/usr/libexec/PlistBuddy -c \"Print :RCTWebSocketExecutor:port\" \"${INFOPLIST_FILE}\")\n\nif nc -w 5 -z localhost $PORT ; then\n  if ! curl -s \"${ORIGIN}:${PORT}/status\" | grep -q \"packager-status:running\" ; then\n    echo \"Port ${PORT} already in use, packager is either not running or not running correctly\"\n    exit 2\n  fi\nelse\n  # open w/t --args doesn't work:\n  # open $SRCROOT/../../packager/launchPackager.command --args 111 222 || echo \"Can't start packager automatically\"\n  osascript -e 'tell app \"Terminal\"\n    do script \"'$SRCROOT'/../../packager/launchPackager.command --origin='${ORIGIN}' --port='${PORT}'\"\n  end tell' || echo \"Can't start packager automatically\"\nfi";


This script is very long, can you extract it to a separate file and call that file from here?

@frantic Sure, can do. What's a good place to place that shell script? This one?: ./scripts.

frantic · 2015-06-15T23:06:44Z

Good work, thank you! But I feel like we should probably do the other way around - have React target be pure and ReactWithPackager do what React target does now. @nicklockwood / @tadeuzagallo - do you think my suggestion makes sence?

ide · 2015-06-15T23:34:13Z

@frantic fwiw I agree with that structure, and would take it a step further by skipping ReactWithPackager and just having SampleApp, UIExplorer, TicTacToe, etc... call the packager from their own pbxproj files.

bparadie · 2015-06-17T15:48:23Z

@frantic @ide Thanks for the feedback. This is one of my weekend projects, please expect some delays etc.
Just to confirm: You suggest switching the default so Info.plist would be supported by the React target and only the ReactWithPackager would launch the packager, correct?

Now, you guys realize that that will break modified old projects. The react-native Get Started section says:

Quick start 
- npm install -g react-native-cli
- react-native init AwesomeProject

All those old AwesomeProjects will be broken if the React target stops launching the packager. Are you ok with that? Can you just add a 'Breaking Changes' section to the next release and we all move on with our lives?

ide · 2015-06-18T20:27:31Z

packager/packager.js

+  command: 'origin',
+  default: 'http://localhost',
+  type: 'string',
+}, {


fyi "origin" (as in specs like CORS) includes the port number

@ide I wanted to model it after HTML's location.origin. But you are right, origin includes the port number. Not sure whether hostname would be a better choice, because it doesn't include the protocol part. Is there a better name than origin? I went through the HTML location properties. But nothing seems to fit...

we could split up protocol and hostname since most of the time (for development) the protocol is http and only the hostname needs to change.

@ide What I am hearing you say is that we should model everything after HTML location and replace origin with hostname and protocol. Is that what you are proposing? If so: I like it!

Yes! protocol, hostname, and port as three separately configurable fields. Could even use Node's url.format() to construct the complete origin string if that helps.

bparadie · 2015-06-21T21:04:38Z

Ok, I've made the following changes:

replaced 'origin' with 'protocol' + 'hostname' as suggested by @ide
moved the debug URL creation code to its own utility function as suggested by @frantic
moved launch mechanics out to launchPackager.command as suggested by @frantic
removed launch script from React project and added launch scripts to examples as suggested by @ide

But:

I had to revert one line from 1acca01, see [UIExplorer] App is broken: Attempted to assign readonly property. (ActivityIndicatorIOS.ios.js) #1654
this change will break old projects.

These are the custom port setting for each example:

Port	Example
8081	UIExplorer
8087	SampleApp
8088	2048
8089	Movies
8090	TicTacToe

Breaking changes: required/optional changes.

If you have an old project and update your react-native module to this version then your old project will not launch the packager, because I removed the launch script from the React project.

Here are the changes that are required/optional...

REQUIRED: Add a launch script

Add a run script to your project with this line

$SRCROOT/../../packager/launchPackager.command ${INFOPLIST_FILE}

OPTIONAL: Use RCTWebSocketExecutorURL

Use RCTWebSocketExecutorURL from RCTUtils.h for creating your jsCodeLocation in AppDelegate.m, i.e.

jsCodeLocation = RCTWebSocketExecutorURL(@"/Examples/SampleApp/index.ios.bundle");

OPTIONAL: Add a RCTWebSocketExecutor dictionary

Add a RCTWebSocketExecutor dictionary for custom settings to your Info.plist, i.e.

Please let me know if this looks good to you.
If so I'll squash/rebase my PR.
Thx!

bparadie · 2015-06-21T21:07:52Z

Hmm, Travis CI build failed. It looks like I need to add the launch scripts to every test targets. Give me a minute!

bparadie · 2015-06-21T22:03:14Z

One remaining problem is that package.json runs packager.sh for the scripts/start and bin/react-native-start those will always launch the packager with the default settings, that is, port=8081.

bparadie · 2015-06-21T22:30:00Z

@frantic I am a little bit puzzled why Travis CI is failing.

2015-06-21 22:27:29.662 [info][tid:com.facebook.React.JavaScript] "Running application "AppEventsTest" with appParams: {"rootTag":1,"initialProps":{}}. __DEV__ === true, development-level warning are ON, performance optimizations are OFF"
2015-06-21 22:27:29.672 [error][tid:com.facebook.React.JavaScript] "Error: Invariant Violation: Application AppEventsTest has not been registered. This is either due to a require() error during initialization or failure to call AppRegistry.registerComponent.
 stack: 
  runApplication          IntegrationTestsA…:36107
  jsCall                  IntegrationTestsA…:7411
  _callFunction           IntegrationTestsA…:7679
  <unknown>               IntegrationTestsA…:7708
  <unknown>               IntegrationTestsA…:7702
  perform                 IntegrationTestsA…:6198
  batchedUpdates          IntegrationTestsA…:14132
  batchedUpdates          IntegrationTestsA…:4741
  <unknown>               IntegrationTestsA…:7701
  applyWithErrorReporter  IntegrationTestsA…:7446
  guardReturn             IntegrationTestsA…:7468
  processBatch            IntegrationTestsA…:7700
 URL: http://localhost:8081/Examples/UIExplorer/UIExplorerIntegrationTests/js/IntegrationTestsApp.includeRequire.runModule.bundle?dev=true
 line: 1762
 message: Invariant Violation: Application AppEventsTest has not been registered. This is either due to a require() error during initialization or failure to call AppRegistry.registerComponent."

I even removed my workaround for #1654 . But that didn't help either. Would you know what's going on here? Thanks!

bparadie · 2015-06-22T02:17:34Z

I can now at least reproduce the problem locally w/t rm -rf node_modules && npm install && ./scripts/objc-test.sh... I had to nuke the node_modules folder first, though. But I am getting the same error with the master branch. Something is not right...

bparadie · 2015-06-22T03:45:25Z

Ok, I found the problem (don't ask how I found it): react-native's package.json requires version 5.4.3 for the babel node nodule. Unfortunately babel's package.json sloppily accepts ^5.4.3 for babel-core. Now, if you start with a fresh clone (as Travis CI does) you'll end up with babel 5.4.3 using babel-core 5.6.2 (which is the latest babel-core version). Here is the problem: babel-core 5.6.2 will break ./scripts/objc-test.sh tests. The version that works is babel-core 5.5.6.

For that reason this PR contains this workaround. In package.json I explicitly request babel-core 5.5.6:

    "babel": "5.4.3",
    "babel-core": "5.5.6",

Would somebody please figure out why babel-core 5.6.2 breaks the build loop?

bparadie · 2015-06-22T03:47:11Z

Almost there:

The last remaining build loop error is: flow server is busy:

Flow server launched for /private/tmp/react-native-x3yOQW0y/EndToEndTest
Spawned flow server (child pid=2838)
Logs will go to /var/folders/vy/rcv48w3j4w79llzf_x6qnvw40000gn/T/flow/zSprivatezStmpzSreact-native-x3yOQW0yzSEndToEndTest.log
The flow server will be ready in a moment.
Error: flow server is busy
Error: flow server is busy
Error: flow server is busy
No errors!
[Info] Loading settings for scheme 'EndToEndTest' ...

That's not really my problem, is it?

frantic · 2015-06-22T16:56:02Z

flow server is busy is just a warning, after that it says No errors and continues the build, so that's not the reason for the failure. This is:

/Users/travis/Library/Developer/Xcode/DerivedData/EndToEndTest-bmjlkbqwxoavxthdezsighobnyjh/Build/Intermediates/EndToEndTest.build/Debug-iphonesimulator/EndToEndTest.build/Script-4CED847C1B3756CA00715E21.sh: line 2: /tmp/react-native-x3yOQW0y/EndToEndTest/../../packager/launchPackager.command: No such file or directory

@DmitrySoshnikov

Reviewed By: @DmitrySoshnikov Differential Revision: D2445071

@vjeux

Summary: I can't find anywhere these are being used by the bridge / native views anymore. I don't think they work anymore. ART has a similar API but uses a different code path. We might as well clean this up. Makes it easier to reason about. @public Reviewed By: @vjeux Differential Revision: D2445353

@sahrens

Summary: @public RCTNetworking currently relies on network responses to include an accurate text encoding, otherwise it is unable to convert the response data to text unless it's encoded as UTF8. See: facebook#1780 (comment) for details. This diff makes use of a new feature in iOS8 to detect the encoding of the text authomatically Reviewed By: @sahrens Differential Revision: D2443446

Jest 0.5.2 ships with a fix for bound functions, which RN exercises. And with this Travis change, the automated tests will install Node 4.x with nvm before running. Test Plan:`nvm use 4 && npm i && npm test` See if Travis tests pass.

@vjeux

Summary: beeper node modules has a `return` statement outside of a function which doesn't parse. To fix it, we mock it. Also, setupEnvPolyfills is no longer being used. Closes facebook#2770 Reviewed By: @svcscm Differential Revision: D2448882 Pulled By: @vjeux

…o webSocketConfig

jsierles · 2015-09-17T21:10:48Z

Just to add my 2c here. This issue is important because it stops developers from getting on board as quickly as possible with debugging from devices. We still have to manually update RCTWebSocketManager.m, and these changes are overwritten when updating React Native. Now that hot reloading is a reality in React Native projects, it's even more important.

bparadie · 2015-09-22T21:49:54Z

Time is up. I am closing this PR.

jsierles · 2015-09-22T23:07:47Z

Hmm, too bad on this one. Given android is out now, maybe it's time to look at a single cross platform solution that doesn't depend on plist files or anything platform-specific. A global config file - maybe package.json?

PaulBGD · 2015-10-09T23:43:19Z

Sad, this is something that would be really useful.

qimingweng · 2015-12-24T17:01:50Z

I've posted this on product pains so it would be great if you could upvote it there and show the react-native team that this is an important feature https://productpains.com/post/react-native/allow-packager-port-to-be-configurable-change-from-8081/

Reviewed By: frantic Differential Revision: D2613142 fb-gh-sync-id: fadcea3d23825420c0412f2e4d8d51c70b0f08ed

facebook-github-bot added the GH Review: review-needed label Jun 7, 2015

bparadie mentioned this pull request Jun 7, 2015

[Debugging] Please make debug port (8081) configurable #1429

Closed

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 Jun 7, 2015

brentvatne assigned amasad Jun 7, 2015

amasad assigned frantic and unassigned amasad Jun 15, 2015

frantic reviewed Jun 15, 2015
View reviewed changes

frantic added GH Review: needs-revision and removed GH Review: review-needed labels Jun 15, 2015

bparadie mentioned this pull request Jun 17, 2015

Adobe flavor of facebook/react-native (read-only) bparadie/react-native#1

Open

ide reviewed Jun 18, 2015
View reviewed changes

facebook-github-bot added GH Review: review-needed and removed GH Review: needs-revision labels Jun 21, 2015

bparadie mentioned this pull request Jun 22, 2015

[MapView] Support for annotation callouts, annotation press, callout presses and pin animation #1247

Closed

Support for configurable debug port (issue 1429)

bd925ac

bparadie force-pushed the webSocketConfig branch from 4caebf4 to bd925ac Compare September 16, 2015 19:34

vjeux and others added 15 commits September 16, 2015 14:50

Update TransformPropTypes.js

ba414c0

Simplify jest config + update to jest 0.5.5 in react-native-github

c1ed130

Reviewed By: @DmitrySoshnikov Differential Revision: D2445071

Get back 100% in sync with fb codebase

40b79e3

Restore unintended sync

62eb728

Bring back build status

f91ea8a

Delete setupEnvPolyfills.js which is no longer being used

7bde0be

Update KnownIssues.md

6c818ef

Update KnownIssues.md

d530c62

Update KnownIssues.md

edb8cb2

Update KnownIssues.md

b1b81cf

Fix more tests

edfb024

Summary: beeper node modules has a `return` statement outside of a function which doesn't parse. To fix it, we mock it. Also, setupEnvPolyfills is no longer being used. Closes facebook#2770 Reviewed By: @svcscm Differential Revision: D2448882 Pulled By: @vjeux

Merge branch 'master' of https://github.com/facebook/react-native int…

1ef1d4b

…o webSocketConfig

bparadie closed this Sep 22, 2015

PublicParadise mentioned this pull request Dec 21, 2015

Is Examples/VideoPlayer completely broken now? TheWidlarzGroup/react-native-video#117

Closed

mosesoak referenced this pull request Jan 18, 2016

Command line flags to start RN apps in web executor mode.

cce9ba8

Reviewed By: frantic Differential Revision: D2613142 fb-gh-sync-id: fadcea3d23825420c0412f2e4d8d51c70b0f08ed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for configurable debug origin/port (issue 1429) #1546

Support for configurable debug origin/port (issue 1429) #1546

bparadie commented Jun 7, 2015

facebook-github-bot commented Jun 7, 2015

facebook-github-bot commented Jun 7, 2015

amasad commented Jun 15, 2015

frantic Jun 15, 2015

bparadie Jun 18, 2015

frantic commented Jun 15, 2015

ide commented Jun 15, 2015

bparadie commented Jun 17, 2015

ide Jun 18, 2015

bparadie Jun 18, 2015

ide Jun 18, 2015

bparadie Jun 19, 2015

ide Jun 19, 2015

bparadie commented Jun 21, 2015

bparadie commented Jun 21, 2015

bparadie commented Jun 21, 2015

bparadie commented Jun 21, 2015

bparadie commented Jun 22, 2015

bparadie commented Jun 22, 2015

bparadie commented Jun 22, 2015

frantic commented Jun 22, 2015

jsierles commented Sep 17, 2015

bparadie commented Sep 22, 2015

jsierles commented Sep 22, 2015

PaulBGD commented Oct 9, 2015

qimingweng commented Dec 24, 2015

Support for configurable debug origin/port (issue 1429) #1546

Support for configurable debug origin/port (issue 1429) #1546

Conversation

bparadie commented Jun 7, 2015

Summary

Changing the port number

Backwards compatibility

Usage in examples

Utility function

facebook-github-bot commented Jun 7, 2015

facebook-github-bot commented Jun 7, 2015

amasad commented Jun 15, 2015

frantic Jun 15, 2015

Choose a reason for hiding this comment

bparadie Jun 18, 2015

Choose a reason for hiding this comment

frantic commented Jun 15, 2015

ide commented Jun 15, 2015

bparadie commented Jun 17, 2015

ide Jun 18, 2015

Choose a reason for hiding this comment

bparadie Jun 18, 2015

Choose a reason for hiding this comment

ide Jun 18, 2015

Choose a reason for hiding this comment

bparadie Jun 19, 2015

Choose a reason for hiding this comment

ide Jun 19, 2015

Choose a reason for hiding this comment

bparadie commented Jun 21, 2015

Breaking changes: required/optional changes.

REQUIRED: Add a launch script

OPTIONAL: Use RCTWebSocketExecutorURL

OPTIONAL: Add a RCTWebSocketExecutor dictionary

bparadie commented Jun 21, 2015

bparadie commented Jun 21, 2015

bparadie commented Jun 21, 2015

bparadie commented Jun 22, 2015

bparadie commented Jun 22, 2015

bparadie commented Jun 22, 2015

frantic commented Jun 22, 2015

jsierles commented Sep 17, 2015

bparadie commented Sep 22, 2015

jsierles commented Sep 22, 2015

PaulBGD commented Oct 9, 2015

qimingweng commented Dec 24, 2015