Unify codepath for retrieveing metro location on Android #42617
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
cortinico
approved these changes
Jan 23, 2024
There was a problem hiding this comment.
In general, I'm fine with this change.
I'd like someone from Metro (cc @huntie or others) to look into this and make sure we are fine with this change.
As you said, this is a breaking change, but I suspect very little people have used this property
Base commit: a13d51f |
|
Thanks, an alternative approach would be to keep these codepaths separate. Meaning that we'd keep separate methods for getting inspector proxy URL and for dev server URLs. I can just make it so that the inspector proxy URL honors the shared preferences and falls back to old behavior otherwise. This would make sense if you plan to split these two servers in the future. I just figured that since the inspector URL flow had a bug that made it read dev server port anyways, it may be better to delete it entirely. |
That's exactly why I'd like someone from DevX to share their thoughts on this |
huntie
approved these changes
Jan 23, 2024
|
@huntie has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
3 tasks
Kudo
added a commit
to expo/expo
that referenced
this pull request
Jan 28, 2024
# Why fix nightlies build error: https://github.com/expo/expo/runs/20922842104 # How since facebook/react-native#42617, the `PackagerConnectionSettings.getInspectorServerHost()` was removed. this pr adds the derived class without `getInspectorServerHost()` for react-native 0.74.
kmagiera
added a commit
to software-mansion/radon-ide
that referenced
this pull request
Mar 19, 2024
…rt settings from shared preferences (#94) This PR implements a workaround for Android builds in order to patch them with a fix that's now merged upstream facebook/react-native#42617 The issue here is that we rely on the mechanism of React Native on Android which reads settings from shared preferences in order to locate metro port. However, React Native Android's implementation uses separate code path for locating the port for the debugger, which normally is the same as the metro port as debugger proxy is one of metro's endpoints. That code path has been broken for a long time as it does not respect the settings you can have in shared preferences, instead it only relies on the port that's provided for the build. This isn't ideal, as we'd need to make a separate build for each device session in the IDE. While this is fixed upstream, we still want to support users on current versions of React Native and Expo. Because in typical workflows, on Android, the java code is not build from source but instead is loaded from pre-built AAR files, we need to override the classes from that package. The way we do it relies on injecting a so-called init script with gradle CLI. The init script sets up an artifact transform which hijacks the JAR file with classes unpacked from react-android dependency, and repackages them while deleting some classes we want to override. On top of that, we add a separate source directory that includes sources for the missing classes. This way we are able to override any internal class from React Android package. We utilize this method to swap implementation of `PackagerConnectionSettings.java` where the only change we're making is to call `getDebugServerHost()` directly from `getInspectorServerHost()` method. This way, the host for debugger uses the same location/port as metro server.
kmagiera
pushed a commit
to software-mansion/radon-ide
that referenced
this pull request
May 21, 2024
Description: some 3rd party libraries, could cause duplication of classes during transformation we performed in configureReactNativeOverrides. This is a solution for RN versions 74 and higher that relays on the fact that we don't need to perform configureReactNativeOverrides for those versions, because the solution were marged upstream. see: facebook/react-native#42617 How to reproduce a bug? - run `npx react-native init your-project` - run `npm i react-native-biometrics` - try to run react-native-ide on android note that react-native-ide will believe that it is not a native change so it you will have to manually clean rebuild every time you add or remove react-native-biometrics. This is the reported error in build logs: <img width="421" alt="Screenshot 2024-05-13 at 02 38 41" src="https://github.com/software-mansion/react-native-ide/assets/159789821/23007fdf-141a-4dbd-a596-7a1c269f5a03"> --------- Co-authored-by: filip131311 <filip.kaminski@swmansiom.com>
Apple0717
added a commit
to Apple0717/radon
that referenced
this pull request
Nov 30, 2024
…rt settings from shared preferences (#94) This PR implements a workaround for Android builds in order to patch them with a fix that's now merged upstream facebook/react-native#42617 The issue here is that we rely on the mechanism of React Native on Android which reads settings from shared preferences in order to locate metro port. However, React Native Android's implementation uses separate code path for locating the port for the debugger, which normally is the same as the metro port as debugger proxy is one of metro's endpoints. That code path has been broken for a long time as it does not respect the settings you can have in shared preferences, instead it only relies on the port that's provided for the build. This isn't ideal, as we'd need to make a separate build for each device session in the IDE. While this is fixed upstream, we still want to support users on current versions of React Native and Expo. Because in typical workflows, on Android, the java code is not build from source but instead is loaded from pre-built AAR files, we need to override the classes from that package. The way we do it relies on injecting a so-called init script with gradle CLI. The init script sets up an artifact transform which hijacks the JAR file with classes unpacked from react-android dependency, and repackages them while deleting some classes we want to override. On top of that, we add a separate source directory that includes sources for the missing classes. This way we are able to override any internal class from React Android package. We utilize this method to swap implementation of `PackagerConnectionSettings.java` where the only change we're making is to call `getDebugServerHost()` directly from `getInspectorServerHost()` method. This way, the host for debugger uses the same location/port as metro server.
Apple0717
added a commit
to Apple0717/radon
that referenced
this pull request
Nov 30, 2024
Description: some 3rd party libraries, could cause duplication of classes during transformation we performed in configureReactNativeOverrides. This is a solution for RN versions 74 and higher that relays on the fact that we don't need to perform configureReactNativeOverrides for those versions, because the solution were marged upstream. see: facebook/react-native#42617 How to reproduce a bug? - run `npx react-native init your-project` - run `npm i react-native-biometrics` - try to run react-native-ide on android note that react-native-ide will believe that it is not a native change so it you will have to manually clean rebuild every time you add or remove react-native-biometrics. This is the reported error in build logs: <img width="421" alt="Screenshot 2024-05-13 at 02 38 41" src="https://github.com/software-mansion/react-native-ide/assets/159789821/23007fdf-141a-4dbd-a596-7a1c269f5a03"> --------- Co-authored-by: filip131311 <filip.kaminski@swmansiom.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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/devicecalls, 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.getInspectorServerHostwithPackagerConnectionSettings.getDebugServerHostwhich 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 where
react_native_dev_server_portwas used insteadreact_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
Test Plan:
--port 9090) while keeping the app runningImportant: 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.