Skip to content

Commit

Permalink
Unify codepath for retrieveing metro location on Android (#42617)
Browse files Browse the repository at this point in the history
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
  • Loading branch information
kmagiera authored and facebook-github-bot committed Jan 25, 2024
1 parent 631b6a1 commit 0ea16fd
Show file tree
Hide file tree
Showing 6 changed files with 2 additions and 27 deletions.
Expand Up @@ -53,15 +53,11 @@ internal object AgpConfiguratorUtils {
fun configureDevPorts(project: Project) {
val devServerPort =
project.properties["reactNativeDevServerPort"]?.toString() ?: DEFAULT_DEV_SERVER_PORT
val inspectorProxyPort =
project.properties["reactNativeInspectorProxyPort"]?.toString() ?: devServerPort

val action =
Action<AppliedPlugin> {
project.extensions.getByType(AndroidComponentsExtension::class.java).finalizeDsl { ext ->
ext.defaultConfig.resValue("integer", "react_native_dev_server_port", devServerPort)
ext.defaultConfig.resValue(
"integer", "react_native_inspector_proxy_port", inspectorProxyPort)
}
}

Expand Down
2 changes: 0 additions & 2 deletions packages/react-native/ReactAndroid/api/ReactAndroid.api
Expand Up @@ -3452,7 +3452,6 @@ public class com/facebook/react/modules/systeminfo/AndroidInfoHelpers {
public static fun getAdbReverseTcpCommand (Landroid/content/Context;)Ljava/lang/String;
public static fun getAdbReverseTcpCommand (Ljava/lang/Integer;)Ljava/lang/String;
public static fun getFriendlyDeviceName ()Ljava/lang/String;
public static fun getInspectorProxyHost (Landroid/content/Context;)Ljava/lang/String;
public static fun getServerHost (Landroid/content/Context;)Ljava/lang/String;
public static fun getServerHost (Ljava/lang/Integer;)Ljava/lang/String;
}
Expand Down Expand Up @@ -3528,7 +3527,6 @@ public abstract class com/facebook/react/packagerconnection/NotificationOnlyHand
public class com/facebook/react/packagerconnection/PackagerConnectionSettings {
public fun <init> (Landroid/content/Context;)V
public fun getDebugServerHost ()Ljava/lang/String;
public fun getInspectorServerHost ()Ljava/lang/String;
public fun getPackageName ()Ljava/lang/String;
public fun setDebugServerHost (Ljava/lang/String;)V
}
Expand Down
6 changes: 0 additions & 6 deletions packages/react-native/ReactAndroid/build.gradle.kts
Expand Up @@ -429,11 +429,6 @@ fun reactNativeDevServerPort(): String {
return value?.toString() ?: "8081"
}

fun reactNativeInspectorProxyPort(): String {
val value = project.properties["reactNativeInspectorProxyPort"]
return value?.toString() ?: reactNativeDevServerPort()
}

fun reactNativeArchitectures(): List<String> {
val value = project.properties["reactNativeArchitectures"]
return value?.toString()?.split(",") ?: listOf("armeabi-v7a", "x86", "x86_64", "arm64-v8a")
Expand Down Expand Up @@ -498,7 +493,6 @@ android {
buildConfigField("int", "EXOPACKAGE_FLAGS", "0")

resValue("integer", "react_native_dev_server_port", reactNativeDevServerPort())
resValue("integer", "react_native_inspector_proxy_port", reactNativeInspectorProxyPort())

testApplicationId = "com.facebook.react.tests.gradle"
testInstrumentationRunner = "androidx.test.runner.AndroidJUnitRunner"
Expand Down
Expand Up @@ -312,7 +312,7 @@ private String getInspectorDeviceUrl() {
return String.format(
Locale.US,
"http://%s/inspector/device?name=%s&app=%s&device=%s",
mPackagerConnectionSettings.getInspectorServerHost(),
mPackagerConnectionSettings.getDebugServerHost(),
Uri.encode(AndroidInfoHelpers.getFriendlyDeviceName()),
Uri.encode(mPackageName),
Uri.encode(getInspectorDeviceId()));
Expand Down Expand Up @@ -491,7 +491,7 @@ public void openDebugger(final ReactContext context, final String errorMessage)
String.format(
Locale.US,
"http://%s/open-debugger?appId=%s&device=%s",
mPackagerConnectionSettings.getInspectorServerHost(),
mPackagerConnectionSettings.getDebugServerHost(),
Uri.encode(mPackageName),
Uri.encode(getInspectorDeviceId()));
Request request =
Expand Down
Expand Up @@ -52,10 +52,6 @@ public static String getAdbReverseTcpCommand(Context context) {
return getAdbReverseTcpCommand(getDevServerPort(context));
}

public static String getInspectorProxyHost(Context context) {
return getServerIpAddress(getInspectorProxyPort(context));
}

// WARNING(festevezga): This RN helper method has been copied to another FB-only target. Any
// changes should be applied to both.
public static String getFriendlyDeviceName() {
Expand All @@ -72,11 +68,6 @@ private static Integer getDevServerPort(Context context) {
return resources.getInteger(R.integer.react_native_dev_server_port);
}

private static Integer getInspectorProxyPort(Context context) {
Resources resources = context.getResources();
return resources.getInteger(R.integer.react_native_dev_server_port);
}

private static String getServerIpAddress(int port) {
// Since genymotion runs in vbox it use different hostname to refer to adb host.
// We detect whether app runs on genymotion and replace js bundle server hostname accordingly
Expand Down
Expand Up @@ -57,10 +57,6 @@ public void setDebugServerHost(String host) {
mPreferences.edit().putString(PREFS_DEBUG_SERVER_HOST_KEY, host).apply();
}

public String getInspectorServerHost() {
return AndroidInfoHelpers.getInspectorProxyHost(mAppContext);
}

public @Nullable String getPackageName() {
return mPackageName;
}
Expand Down

0 comments on commit 0ea16fd

Please sign in to comment.