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

Inject dev machine IP on Android and improve error message when connection fails #49166

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

hrastnik
Copy link

@hrastnik hrastnik commented Feb 4, 2025

Summary:

I've implemented a feature that automatically bundles the Metro Bundler's IP address into Android builds. This change aligns the Android development experience with iOS, allowing the app to maintain a connection to the Metro Bundler even when disconnected from USB.

Currently, in iOS builds, the IP address of the computer running the Metro Bundler is automatically bundled into the app, ensuring seamless connectivity even when the device is disconnected from USB. In contrast, Android developers must manually input the IP address if the USB connection is lost, which can be tedious and error-prone.

More info in discussion thread: react-native-community/discussions-and-proposals#870

I anticipate that a change where making IP the default method of connection will result in a lot of people running into issues where they can't connect to Metro server (for example, if they're on a different network, or they disable wifi). So I also changed the default error message you get in case the app can't connect to the bundler and updated the "Change Bundle Location" dev menu.

The previous error message

Unable to load script. Make sure you're either
running Metro (run 'npx react-native start') or
that your bundle 'RNTesterApp.android.bundle' is
packaged correctly for release.

was changed to:

Unable to load script.

Make sure you're running Metro (npx react-native start)
or that your bundle 'RNTesterApp.android.bundle' is
packaged correctly for release.

The device must be on the same WiFi as your laptop to
connect to Metro.

To use USB instead, shake the device to open the dev
menu and set the bundler location to 'localhost: 8081'
and run:
  adb reverse tcp:8081 tcp:8081

image

And the new dev menu UI looks like this:
image

The two buttons with "10.0.2.2:8081" and "localhost:8081" are suggestions which when tapped fill the input with the text from the button. The first button suggests the IP of the development machine, and the second one is hardcoded to localhost:8081.

Changelog:

[ANDROID] [CHANGED] - Automatically use Metro bundler IP address when installing apps on Android

Test Plan:

I've tested the implementation on a physical device and on emulator and it's working solid. However, I would invite further testing in order to catch possible edge cases.

I've recorded common scenarios

Scenario 1:
Device doesn't have the app installed.
We connect the device via USB, install the app and open it.
Device is on the same network as the dev machine.
Bundler location is by default set to the IP of the dev machine.
When starting app, the app is able to connect to the dev machine and download the bundle.

Scenario 2:
Device doesn't have the app installed.
Wi-Fi is turned off on the device but device is connected via USB
We install the app and open it.
Bundler location is by default set to the IP of the dev machine.
When starting app, the app is not able to connect to the dev machine and shows the error message.
After opening the dev menu we see that the IP is set to the IP of the dev machine.
We click the "localhost" option in the dev menu and click apply
After that the app is able to connect to the dev machine and download the bundle (via USB) since the traffic is forwarded using adb reverse.

Notes:
When we set an IP in the dev menu, the app will persist it.
If we connect the device via USB and reinstall the app the persisted data stays the same, so the previously set IP will be used.
However, the IP of the dev machine will be displayed as an option in the dev menu.

rec-1-not-on-same-wifi.mp4
rec-2-same-network.mp4

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Feb 4, 2025
@hrastnik hrastnik requested a review from cortinico February 5, 2025 22:16
@cortinico cortinico requested a review from huntie February 6, 2025 09:48
Copy link
Member

@huntie huntie left a comment

Choose a reason for hiding this comment

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

@hrastnik Thanks!

Would you be able to include a video recording in the test plan? Just so we can better understand the working flow :)

@hrastnik
Copy link
Author

@huntie @cortinico I've recorded the most common scenarios and added to test plan. And I've merged the proposed wording changes.

@facebook-github-bot
Copy link
Contributor

@huntie has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

cortinico
cortinico previously approved these changes Feb 14, 2025
Comment on lines 91 to 97
"Unable to load script.\n\n"
"Make sure you're running Metro or that your "
"bundle '", assetName, "' is packaged correctly for release.\n\n"
"The device must be on the same Wi-Fi network as your laptop to connect to Metro.\n\n"
"To use USB instead, shake the device to open the Dev Menu and set "
"the bundler location to \"localhost:8081\" and run:\n"
" adb reverse tcp:8081 tcp:8081"));
Copy link
Contributor

Choose a reason for hiding this comment

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

nice :)

Copy link
Member

Choose a reason for hiding this comment

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

Ah, since we're here still, can we substitute "laptop" for "computer" too?

Also I'm not 100% on how line wrapping should work in Kotlin, but maybe these could be rebalanced? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

@huntie when you import it, the formatter will take care of it

Copy link
Author

Choose a reason for hiding this comment

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

I replaced "laptop" with "computer". Not sure about how to rebalance this better though.

@cortinico
Copy link
Contributor

This will have to be tested on top of:

And making sure that running:

./gradlew build --configuration-cache

twice is not building anything. I suspect not, but let's do it before we merge it

@facebook-github-bot
Copy link
Contributor

@huntie has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@cortinico
Copy link
Contributor

Hey @hrastnik
I've tested this and it's not breaking Gradle Config Cache (which is good).

However, I'm a bit concerned about this as getting the machine IP by default AND using it as server address is a risky solution.

For example say I'm on a hotel wifi, and I can't find a way to connect the phone to the metro running on my laptop. And I don't know if the hotel wifi is blocking TCP connections between hosts OR if I have the macos firewall enabled OR if my employer is blocking incoming TCP connections on my machine.

So we end up forcing users to 'debug' their network setup, just to avoid an adb reverse command which they're used to.

I believe this should be rolled out more gradually.
As a bare minimum:

  1. Default to localhost:8081 always, and let the user select the machine IP address we collected at build time if they wish.
  2. Add a Gradle property to toggle if the build time IP should be used instead (that prop should be off by default)

@hrastnik
Copy link
Author

hrastnik commented Mar 1, 2025

@cortinico I understand the concern, but I still feel like using the IP as the server address should be the default.

  1. It matches the iOS behaviour.
  2. Most devs are developing RN apps either at home or work, with stable networks.
  3. In case the network is unstable for any reason, and the app fails to connect to the Metro server, there are instruction on how to connect via USB.

However, if you feel like it's still too risky, I can update my PR and add a gradle property that would control the behaviour.

@hrastnik
Copy link
Author

hrastnik commented Mar 7, 2025

@cortinico Any updates on this? Should I add the Gradle property, or is there any chance we can roll with it as is?

@cortinico
Copy link
Contributor

@cortinico Any updates on this? Should I add the Gradle property, or is there any chance we can roll with it as is?

I personally tested this on both our corp wifi (at Meta) and in a hotel wifi when I was travelling, and neither scenarios worked out of the box.

I'm really concerned that if we ship this as it is by default, we're going to create more frustration than benefits

I'd suggest we make it optional and potentially change it to be a default in a later release

@cortinico cortinico dismissed their stale review March 7, 2025 15:12

See my previous comments

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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants