Android Fix for 9145: No longer hard code build port #23616

nhunzaker · 2019-02-24T03:01:03Z

Summary

Problem

According to #9145, the --port setting is not respected when executing react-native run-android. The templates that report things like what port the dev server runs on are hard coded as well.

Solution

This commit replaces the hardcoded instances of port 8081 on Android with a build configuration property. This allows setting of the port React Native Android connects to for the local build server.

For this change to work, there must also be an update to the react native CLI to pass along this setting:

react-native-community/cli@master...nhunzaker:9145-android-no-port-hardcode-cli

To avoid some noise on their end, I figured I wouldn't submit a PR until it's this approach is deemed workable.

Changelog

[Android][fixed] - react-native run-android --port <x> correctly connects to dev server and related error messages display the correct port

Test Plan

Preparing a test case for this is pretty involved, but:

Clone https://github.com/nhunzaker/react-native-cli/tree/9145-android-no-port-hardcode-cli
Install dependencies for this project
Clone https://github.com/nhunzaker/react-native/tree/9145-android-no-port-hardcode
Install dependencies for this project
Create a new RN project
Link this branch to the new project following The Build From Source Instructions

(for future PR's, I'd love figure out a better flow for reproducing fixes)

Then from the new project, execute:

yarn start --port 3000

And in another terminal window:

node ../react-native-cli/packages/cli/ run-android --port 3000

You can also do this from gradle via:

./gradlew :app:installDebug -PdebugHostPort=3001

Error messages are also updated, so you'll see the correct port if the server is down:

nhunzaker · 2019-02-24T03:02:05Z

ReactAndroid/src/main/java/com/facebook/react/common/BUCK

@@ -33,6 +33,7 @@ rn_android_build_config(
    package = "com.facebook.react",
    values = [
        "boolean IS_INTERNAL_BUILD = true",
+        "int DEBUG_SERVER_HOST_PORT = 8081",


I had to do this to get unit tests to pass. Not sure if this is the best approach.

nhunzaker · 2019-02-24T03:03:13Z

ReactAndroid/src/main/java/com/facebook/react/common/DebugServerException.java

@@ -28,8 +30,8 @@
      "\u2022 Ensure that the packager server is running\n" +
      "\u2022 Ensure that your device/emulator is connected to your machine and has USB debugging enabled - run 'adb devices' to see a list of connected devices\n" +
      "\u2022 Ensure Airplane Mode is disabled\n" +
-      "\u2022 If you're on a physical device connected to the same machine, run 'adb reverse tcp:8081 tcp:8081' to forward requests from your device\n" +
-      "\u2022 If your device is on the same Wi-Fi network, set 'Debug server host & port for device' in 'Dev settings' to your machine's IP address and the port of the local dev server - e.g. 10.0.1.1:8081\n\n";
+      "\u2022 If you're on a physical device connected to the same machine, run 'adb reverse tcp:" + DEBUG_SERVER_HOST_PORT + " tcp:" + DEBUG_SERVER_HOST_PORT + "' to forward requests from your device\n" +


I really want to use AndroidInfoHelpers.getAdbReverseTcpCommand() here, but I wasn't sure if modules in common should depend on other modules.

Salakar · 2019-02-25T03:18:04Z

@nhunzaker thanks for getting this started 🎉

It looks like you've submitted your changes to ReactAndroid/build.gradle. This will only work when building an app from RN sources (as per your Test Plan) - RN releases for Android are distributed as an Android Library (aar format).

You'd need to make your changes in /react.gradle in the root of the repository instead for this to work in release versions. Probably a combination of changes to both files will be needed.

nhunzaker · 2019-02-25T17:53:48Z

@Salakar ahh cool. Thanks! I think I finally got a flow down where I can build the archive and link it to a project. Sure enough, the port isn't passed through. I'll try to figure it out!

ReactAndroid/src/main/java/com/facebook/react/common/DebugServerException.java

nhunzaker · 2019-02-26T05:42:03Z

@Salakar I was able to make headway by using a configurable integer res value:

// ReactNative/build.gradle
 resValue "integer", "REACT_NATIVE_DEV_SERVER_PORT", "8081"

This makes it easy to add an overide in react.gradle:

// react.gradle
 def debugHostPort(String defaultValue) {
    def value = project.getProperties().get("debugHostPort")
    return value != null ? value : defaultValue
}

android {
    buildTypes.all {
        resValue "integer", "REACT_NATIVE_DEV_SERVER_PORT", debugHostPort("8081")
    }
}

I first looked at manifest placeholders, but they're annoying because they can easily be overridden by other use cases, like if you assign over them with manifestPlaceholders = [....]

Resource values were also reasonable to figure out in Buck, which I'm not very familiar with.

The drawback is that the utilities that access the port also need access to context, but I think it's manageable. It's only a problem right now for the error message the suggests adb reverse tcp:<port>.

What do you think about this approach?

nhunzaker · 2019-02-27T18:02:31Z

I updated DebugServerException to report the correct port. I think this PR is ready for review again.

nhunzaker · 2019-02-27T18:07:57Z

ReactAndroid/build.gradle

@@ -286,6 +291,9 @@ android {

        buildConfigField("boolean", "IS_INTERNAL_BUILD", "false")
        buildConfigField("int", "EXOPACKAGE_FLAGS", "0")
+
+        resValue "integer", "react_native_dev_server_port", reactNativeDevServerPort("8081")


This isn't strictly necessary, but useful if you link the ReactNative module directly.

nhunzaker · 2019-03-07T23:45:28Z

Just following up here. Without putting pressure to review this, I wanted to make sure things are easy to confirm, and I really wanted an easier way to verify changes for future fixes, so I made a repo to help with that:

https://github.com/nhunzaker/ReactNativeTestPlan

Assuming you have buck installed already, I think all you have to do is:

git clone git@github.com:nhunzaker/ReactNativeTestPlan.git
cd ReactNativeTestPlan

cd react-native
git remote add nhunzaker git@github.com:nhunzaker/react-native.git
git fetch nhunzaker
git checkout nhunzaker/9145-android-no-port-hardcode

cd ../react-native-cli
git remote add nhunzaker git@github.com:nhunzaker/react-native-cli.git
git fetch nhunzaker
git checkout nhunzaker/9145-android-no-port-hardcode-cli

cd .. # back to project root

yarn install
yarn build-react-native

There is a more comprehensive README for setting things up from scratch, and you might need to verify that the NDK gets connected correctly, but it demonstrates the fix.

Then confirm the change with:

yarn start --port 4000
yarn run-android --port 4000

The Android app should build and connect to port 4000

I'm not sure if you all have something like this already, but it was really helpful for me.

grabbou · 2019-03-19T11:48:42Z

Thanks for sending this PR! I think it's really important and valuable that you have submitted a fix for that after the issue being open for so long.

cpojer · 2019-04-02T18:09:51Z

What's the status of this PR?

nhunzaker · 2019-04-02T19:03:48Z

This is ready for review, but I'm worried the complicated test plan is making review difficult.

In the short term, I'd be happy to address any feedback about the code itself.

cpojer

This looks good to me. @nhunzaker can you rebase it so I can land this?

This commit removes the hardcoded instances of port 8081 on Android with a build configuration property. This allows setting of the port React Native Android connects to for the local build server. For this change to work, there must also be an update to the react native CLI to pass along this setting. Related issues: facebook#9145

- Also adds new buck module for integer resource for testing

This commit downcases REACT_NATIVE_DEV_SERVER_PORT to match other string resources and sets up the debug server to correctly report errors.

nhunzaker · 2019-06-04T21:08:22Z

ReactAndroid/src/main/java/com/facebook/react/modules/systeminfo/AndroidInfoHelpers.java

@@ -23,7 +25,7 @@

  public static final String METRO_HOST_PROP_NAME = "metro.host";

-  private static final int DEBUG_SERVER_HOST_PORT = 8081;
+  private static final int DEBUG_SERVER_HOST_PORT = ReactBuildConfig.DEBUG_SERVER_HOST_PORT;
  private static final int INSPECTOR_PROXY_PORT = 8081;


@cpojer I noticed these ports are now the same on master. I wonder if this should now be:

private static final int DEBUG_SERVER_HOST_PORT = ReactBuildConfig.DEBUG_SERVER_HOST_PORT; private static final int INSPECTOR_PROXY_PORT = ReactBuildConfig.DEBUG_SERVER_HOST_PORT;

Any idea?

Can we use the same config by default but still allow it to be customized separately? I think that’s ideal.

Yep. That makes sense to me.

Confused myself during the rebase. This actually was removed and replaced with a resource value.

I can definitely do the same with the inspector proxy port. Just need to trouble shoot some issues running tests from a fresh setup.

nhunzaker · 2019-06-04T21:12:16Z

@cpojer Updated! Just had question about the update to the inspector proxy port (https://github.com/facebook/react-native/pull/23616/files#r290498124)

Also looks like a lot of CI checks are failing. I'll look into that next.

cpojer · 2019-06-04T21:24:29Z

Sounds good! Let me know once CI is passing (don’t worry if the same job fails on master) and then I can merge it. We can update the CLI in a separate commit later once you land those changes there.

…v port

nhunzaker · 2019-06-05T05:12:29Z

ReactAndroid/build.gradle

@@ -289,6 +299,10 @@ android {

        buildConfigField("boolean", "IS_INTERNAL_BUILD", "false")
        buildConfigField("int", "EXOPACKAGE_FLAGS", "0")
+
+        resValue "integer", "react_native_dev_server_port", reactNativeDevServerPort()
+        resValue "integer", "react_native_inspector_proxy_port", reactNativeInspectorProxyPort()


Updated. This now lets you set the inspector port using:

./gradlew :app:installDebug -PreactNativeDevServerPort=3000

By default it uses the dev server port.

cpojer

Let's ship it.

facebook-github-bot

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

react-native-bot · 2019-06-05T13:20:04Z

This pull request was successfully merged by @nhunzaker in 995b4d3.

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

Summary: ### Problem According to #9145, the `--port` setting is not respected when executing `react-native run-android`. The templates that report things like what port the dev server runs on are hard coded as well. ### Solution This commit replaces the hardcoded instances of port 8081 on Android with a build configuration property. This allows setting of the port React Native Android connects to for the local build server. For this change to work, there must also be an update to the react native CLI to pass along this setting: react-native-community/cli@master...nhunzaker:9145-android-no-port-hardcode-cli To avoid some noise on their end, I figured I wouldn't submit a PR until it's this approach is deemed workable. ## Changelog [Android][fixed] - `react-native run-android --port <x>` correctly connects to dev server and related error messages display the correct port Pull Request resolved: #23616 Differential Revision: D15645200 Pulled By: cpojer fbshipit-source-id: 3bdfd458b8ac3ec78290736c9ed0db2e5776ed46

nhunzaker · 2019-06-07T19:09:02Z

Just following up, I have sent along the accompanying PR for the CLI here:
react-native-community/cli#421

Though I hit some trouble with some changes for the 2.0 release (described over there). I'll keep things moving along over on the cli repo!

Summary: ### Problem According to facebook#9145, the `--port` setting is not respected when executing `react-native run-android`. The templates that report things like what port the dev server runs on are hard coded as well. ### Solution This commit replaces the hardcoded instances of port 8081 on Android with a build configuration property. This allows setting of the port React Native Android connects to for the local build server. For this change to work, there must also be an update to the react native CLI to pass along this setting: react-native-community/cli@master...nhunzaker:9145-android-no-port-hardcode-cli To avoid some noise on their end, I figured I wouldn't submit a PR until it's this approach is deemed workable. ## Changelog [Android][fixed] - `react-native run-android --port <x>` correctly connects to dev server and related error messages display the correct port Pull Request resolved: facebook#23616 Differential Revision: D15645200 Pulled By: cpojer fbshipit-source-id: 3bdfd458b8ac3ec78290736c9ed0db2e5776ed46

Summary: With the current ways metro location is determined, when we want to use a different metro port this requires app to be rebuild as the port and location are stored in resource file that gets compiled to R.class. The only way to avoid app rebuild due to a port change is to use shared preferences that can be accessed from dev menu, where metro URL can be specified. However, due to a separate code-paths for retrieving bundle location and for `/inspector/device` calls, the setting only applies to the former. As a consequence, you can change metro URL in the shared preferences, but debugging would only work if you use the default port or you rebuild the app with the correct port number. This PR removes the separate code-path for retrieving inspector URL including all the dependencies scattered across different files including the gradle plugin. We then replace calls to `PackagerConnectionSettings.getInspectorServerHost` with `PackagerConnectionSettings.getDebugServerHost` which respects the shared preferences and other possible ways of configuring the port. I decided to remove the separate inspector URL code path, as the resource value for inspector port added in #23616 was never functioning properly due to a bug. In the said PR introduced a bug in [AndroidInfoHelpers.java](https://github.com/facebook/react-native/blob/a13d51ff1c38ea85e59f4215563c0dd05452f670/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/modules/systeminfo/AndroidInfoHelpers.java#L77) where `react_native_dev_server_port` was used instead `react_native_inspector_proxy_port`. As a result the added resource value was never read. This can be potentially a breaking change as I'm removing some public methods. However I think it is unlikely anyone relied on said methods. As a part of this PR I'm also changing occurences of removed methods from ReactAndroid.api – I don't know how to test those changes since I don't understand how this file is used as it doesn't have any references in public code. ## Changelog: [ANDROID] [FIXED] - Make Android respect metro location from shared preferences for the debugger workflow Pull Request resolved: #42617 Test Plan: 1. Run android app on emulator using default port 2. Check the debugger works when using "Open Debugger" option from dev menu 3. Restart metro with custom port (`--port 9090`) while keeping the app running 4. Open dev menu, click "Settings" then "Debug server host & port", put "10.0.2.2:9090" there 5. Reload the app 6. Before this change things like hot reload would continue to work while "Open Debugger" option would do nothing 7. After this change both reloading and debugging will work Important: I haven't tested changes made to ReactAndroid.api as I don't know what this files is used for with no references in the codebase. Reviewed By: cortinico Differential Revision: D53010023 Pulled By: huntie fbshipit-source-id: cc8b9c5c7e834ec9ea02b1ed5acf94f04f7b7116

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 Feb 24, 2019

nhunzaker mentioned this pull request Feb 24, 2019

Port is hard coded to 8081 on iOS, Android templates #9145

Closed

nhunzaker commented Feb 24, 2019

View reviewed changes

react-native-bot added the Platform: Android Android applications. label Feb 24, 2019

nhunzaker commented Feb 26, 2019

View reviewed changes

ReactAndroid/src/main/java/com/facebook/react/common/DebugServerException.java Show resolved Hide resolved

nhunzaker commented Feb 27, 2019

View reviewed changes

hramos added PR: Includes Changelog DX Issues concerning how the developer experience can be improved. labels Mar 6, 2019

grabbou requested a review from dulmandakh March 19, 2019 11:49

hramos removed Includes Changelog labels May 6, 2019

cpojer approved these changes Jun 4, 2019

View reviewed changes

nhunzaker added 7 commits June 4, 2019 14:03

Pass port through resource integer value

4489079

Pass dev server port through integer res value

b68f88b

- Also adds new buck module for integer resource for testing

Revert trailing comma ommission

2a47e94

Revert some whitespace and module styles

1331f8a

Downcase port resource. Pass port into DebugServerException

5fb254e

This commit downcases REACT_NATIVE_DEV_SERVER_PORT to match other string resources and sets up the debug server to correctly report errors.

Update ReactNative build.gradle to use new dev server port property

d3f325d

nhunzaker force-pushed the 9145-android-no-port-hardcode branch from cd17376 to d3f325d Compare June 4, 2019 21:05

nhunzaker commented Jun 4, 2019

View reviewed changes

nhunzaker added 2 commits June 4, 2019 21:14

Remove unused build config field

657c2b4

Allow inspector proxy port to be configured as well, defaulting to de…

9aeea1e

…v port

nhunzaker commented Jun 5, 2019

View reviewed changes

cpojer approved these changes Jun 5, 2019

View reviewed changes

facebook-github-bot approved these changes Jun 5, 2019

View reviewed changes

facebook-github-bot closed this in 995b4d3 Jun 5, 2019

react-native-bot added the Merged This PR has been merged. label Jun 5, 2019

nhunzaker mentioned this pull request Jun 7, 2019

feat: Pass along CLI port configuration for Android react-native-community/cli#421

Merged

brentvatne mentioned this pull request Oct 16, 2019

Re-enable development tools for Android expo/expo#5988

Closed

kmagiera mentioned this pull request Jan 23, 2024

Unify codepath for retrieveing metro location on Android #42617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android Fix for 9145: No longer hard code build port #23616

Android Fix for 9145: No longer hard code build port #23616

nhunzaker commented Feb 24, 2019 •

edited

Loading

nhunzaker Feb 24, 2019

nhunzaker Feb 24, 2019

Salakar commented Feb 25, 2019 •

edited

Loading

nhunzaker commented Feb 25, 2019

nhunzaker commented Feb 26, 2019

nhunzaker commented Feb 27, 2019

nhunzaker Feb 27, 2019

nhunzaker commented Mar 7, 2019 •

edited

Loading

grabbou commented Mar 19, 2019

cpojer commented Apr 2, 2019

nhunzaker commented Apr 2, 2019

cpojer left a comment

nhunzaker Jun 4, 2019 •

edited

Loading

cpojer Jun 4, 2019

nhunzaker Jun 4, 2019

nhunzaker Jun 5, 2019

nhunzaker commented Jun 4, 2019

cpojer commented Jun 4, 2019

nhunzaker Jun 5, 2019

cpojer left a comment

facebook-github-bot left a comment

react-native-bot commented Jun 5, 2019

nhunzaker commented Jun 7, 2019

Android Fix for 9145: No longer hard code build port #23616

Android Fix for 9145: No longer hard code build port #23616

Conversation

nhunzaker commented Feb 24, 2019 • edited Loading

Summary

Problem

Solution

Changelog

Test Plan

nhunzaker Feb 24, 2019

Choose a reason for hiding this comment

nhunzaker Feb 24, 2019

Choose a reason for hiding this comment

Salakar commented Feb 25, 2019 • edited Loading

nhunzaker commented Feb 25, 2019

nhunzaker commented Feb 26, 2019

nhunzaker commented Feb 27, 2019

nhunzaker Feb 27, 2019

Choose a reason for hiding this comment

nhunzaker commented Mar 7, 2019 • edited Loading

grabbou commented Mar 19, 2019

cpojer commented Apr 2, 2019

nhunzaker commented Apr 2, 2019

cpojer left a comment

Choose a reason for hiding this comment

nhunzaker Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

cpojer Jun 4, 2019

Choose a reason for hiding this comment

nhunzaker Jun 4, 2019

Choose a reason for hiding this comment

nhunzaker Jun 5, 2019

Choose a reason for hiding this comment

nhunzaker commented Jun 4, 2019

cpojer commented Jun 4, 2019

nhunzaker Jun 5, 2019

Choose a reason for hiding this comment

cpojer left a comment

Choose a reason for hiding this comment

facebook-github-bot left a comment

Choose a reason for hiding this comment

react-native-bot commented Jun 5, 2019

nhunzaker commented Jun 7, 2019

nhunzaker commented Feb 24, 2019 •

edited

Loading

Salakar commented Feb 25, 2019 •

edited

Loading

nhunzaker commented Mar 7, 2019 •

edited

Loading

nhunzaker Jun 4, 2019 •

edited

Loading