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.

Sign up for GitHub

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

Jump to bottom

[android] Propose to connect metro server programmatically #31842

Closed
wants to merge 5 commits into from
Closed

[android] Propose to connect metro server programmatically #31842

wants to merge 5 commits into from

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Jul 9, 2021 •
edited
Loading

Summary

same as #31828 but for android. by calling setNonPersistentDebugServerHost("192.168.123.123:12345"), we could control the metro connection programmatically.

this pr also fix an issue for inspector connection does not honor custom packager setting. when user uses a custom metro server address in dev menu, hermes inspector cannot work in this case.

Changelog

[Internal] [Android] [Added] - Propose to connect metro server programmatically
[Android] [Fixed] - Fix Hermes inspector does not work for custom metro server address.

Test Plan

  1. run metro server at 8082 port by yarn start --port 8082
  2. chrome://inspect monitor localhost:8082 port
  3. launch rntester with hermes and see if chrome inspector can find rntester app.

@facebook-github-bot facebook-github-bot added Contributor A React Native contributor. CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Jul 9, 2021
@analysis-bot
Copy link

analysis-bot commented Jul 9, 2021 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: f58c496

@analysis-bot
Copy link

analysis-bot commented Jul 9, 2021 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 9,120,131 +830,621
android hermes armeabi-v7a 8,646,166 +792,524
android hermes x86 9,559,482 +902,917
android hermes x86_64 9,525,653 +909,950
android jsc arm64-v8a 10,762,957 +971,725
android jsc armeabi-v7a 9,680,247 +928,085
android jsc x86 10,797,703 +1,057,551
android jsc x86_64 11,405,377 +1,064,379

Base commit: f58c496

@brentvatne brentvatne added the p: Expo Partner: Expo label Jul 9, 2021
@yungsters yungsters added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 10, 2021
@yungsters yungsters mentioned this pull request Sep 10, 2021
Closed
@facebook-github-bot
Copy link
Contributor

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

@pull-bot
Copy link

pull-bot commented Sep 13, 2021 •
edited
Loading

Fails
🚫

❗ Base Branch - The base branch for this PR is something other than master. Are you sure you want to target something other than the master branch?

Generated by 🚫 dangerJS against fb49c12

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Oct 1, 2021
yungsters
yungsters requested changes Oct 19, 2021
View reviewed changes
Copy link
Contributor

@yungsters yungsters left a comment

Choose a reason for hiding this comment

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

Passing along internal review feedback from @makovkastar.

...tAndroid/src/main/java/com/facebook/react/packagerconnection/PackagerConnectionSettings.java Outdated
@@ -54,10 +54,21 @@ public String getDebugServerHost() {
}

public void setDebugServerHost(String host) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know if this class can be accessed from multiple threads? Since we are introducing a non-final class member variable, we need to make sure it's accessed through proper synchronisation if that's the case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

update with synchronized setter/getter because the getter may be from background thread.

...tAndroid/src/main/java/com/facebook/react/packagerconnection/PackagerConnectionSettings.java Outdated
mPreferences.edit().putString(PREFS_DEBUG_SERVER_HOST_KEY, host).apply();
}

public void setDebugServerHostWithoutPersisting(final String host) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This new method is not used in this diff, why is it being introduced?

Copy link
Contributor

Choose a reason for hiding this comment

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

Let's also remove final from the argument definition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This new method is not used in this diff, why is it being introduced?

it's to change metro server programmatically. for expo client, we need this to load js bundles from other servers.

...tAndroid/src/main/java/com/facebook/react/packagerconnection/PackagerConnectionSettings.java Outdated
Comment on lines 66 to 67
// Check host setting first. If empty try to detect emulator type and use default
// hostname for those
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment seems to be copy-pasted from getDebugServerHost() and is misleading. We are not trying to detect the emulator type here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated, please let me know if the new comments make sense or not.

@Kudo
Copy link
Contributor Author

Kudo commented Oct 26, 2021

thanks for the neat review comments. please let me know how you think for the updated changes.

@github-actions GitHub Actions
Copy link

This PR is stale because it has been open 180 days with no activity. Remove stale label or comment or this will be closed in 7 days.

@github-actions github-actions bot added the Stale There has been a lack of activity on this issue and it may be closed soon. label Feb 27, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 5, 2024

This PR was closed because it has been stalled for 7 days with no activity.

@github-actions github-actions bot closed this Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@luluwu2032 luluwu2032 luluwu2032 left review comments

@yungsters yungsters yungsters requested changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Contributor A React Native contributor. p: Expo Partner: Expo Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Stale There has been a lack of activity on this issue and it may be closed soon. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Kudo @analysis-bot @facebook-github-bot @pull-bot @yungsters @luluwu2032 @brentvatne @react-native-bot