fix(react-native): pass the protocol from bundle URL to HMR client on Android#48998
Closed
byCedric wants to merge 2 commits intofacebook:mainfrom
Closed
fix(react-native): pass the protocol from bundle URL to HMR client on Android#48998byCedric wants to merge 2 commits intofacebook:mainfrom
byCedric wants to merge 2 commits intofacebook:mainfrom
Conversation
Contributor
|
@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Contributor
Collaborator
|
This pull request was successfully merged by @byCedric in 32fe244 When will my fix make it into a release? | How to file a pick request? |
3 tasks
cipolleschi
pushed a commit
that referenced
this pull request
Feb 3, 2025
… Android (#48998) Summary: This is another attempt at fixing the Android HMR client for HTTPS proxied Metro instances. The previous one unintentionally [caused the following error](#48970 (comment)): ``` java.lang.AssertionError: Method overloading is unsupported: com.facebook.react.devsupport.HMRClient#setup ``` This PR removes the overloading, and only adds the `scheme` property as a parameter to the existing `.setup` method. Aligning with the exact behavior we have on iOS. The alternative fix, which should NOT be backward breaking (if this is) - is to move this "infer the protocol from the bundle URL" to the JS side of the HMR client. Where we don't just always default to `http`, but instead default to `https IF port === 443, otherwise http`. It's a bit more hacky, but shouldn't cause any other issues. _**Ideally**_, we have the same working behavior on both Android and iOS without workarounds. <details><summary>Alternative workaround</summary> See [this change](main...byCedric:react-native:patch-2). <img width="1179" alt="image" src="https://github.com/user-attachments/assets/47c365bc-6df8-43e6-ad7d-5a667e350cd4" /> </details> See full explanation on #48970 > We've noticed that the HMR on Android doesn't seem to be connecting when using a HTTPS-proxied Metro instance, where the proxy is hosted through Cloudflare. This is only an issue on Android - not iOS - and likely caused by the HMR Client not being set up properly on Android. > >- On Android, we run `.setup('android', <bundleEntryPath>, <proxiedMetroHost>, <proxiedMetroPort>, <hmrEnabled>)` in the [**react/devsupport/DevSupportManagerBase.java**](https://github.com/facebook/react-native/blob/53d94c3abe3fcd2168b512652bc0169956bffa39/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerBase.java#L689-L691) file. >- On iOS, we run `[self.callableJSModules invokeModule:@"HMRClient" method:@"setup" withArgs:@[ RCTPlatformName, path, host, RCTNullIfNil(port), @(isHotLoadingEnabled), scheme ]];` in the [**React/CoreModules /RCTDevSettings.mm**](https://github.com/facebook/react-native/blob/53d94c3abe3fcd2168b512652bc0169956bffa39/packages/react-native/React/CoreModules/RCTDevSettings.mm#L488-L491) file. > >Notice how Android does not pass in the scheme/protocol of the bundle URL, while iOS actually does? Unfortunately, because the default protocol (`http`) mismatches on Android when using HTTPS proxies, we actually try to connect the HMR client over `http` instead of `https` - while still using port 443 - which is rejected by Cloudflare's infrastructure even before we can redirect or mitigate this issue. And the rejection is valid, as we basically try to connect on `http://<host>:443` (the source URL is `https`, so the port is infered as `443`). > >This change adds scheme propagation to Android, exactly like we do on iOS for the HMR Client. ## Changelog: [ANDROID] [FIXED] Pass the bundle URL protocol when setting up HMR client on Android <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #48998 Test Plan: See full explanation on #48970 > It's a little bit hard to test this out yourself, since you'd need a HTTPS-based proxy and reject HTTP connections for HTTPS/WSS Websocket requests. > >You can set this up through: >- `bun create expo@latest ./test-app` >- `cd ./test-app` >- `touch .env` >- Set `EXPO_PACKAGER_PROXY_URL=https://<proxied-metro-hostname>` in **.env** >- Set `REACT_NATIVE_PACKAGER_HOSTNAME=<proxied-metro-hostname>` in **.env** >- `bun run start` > >Setting both these envvars, the bundle URL in the manifest is set to `https://...` - which triggers this HMR issue on Android. You can validate the **.env** setup through: > >```bash >curl "http://localhost:8081" -H "expo-platform: android" | jq .launchAsset.url >``` > >This should point the entry bundle URL towards the `EXPO_PACKAGER_PROXY_URL`. Reviewed By: cortinico Differential Revision: D68768351 Pulled By: javache fbshipit-source-id: 49bf1dc60f11b2af6e57177141270632d62ab564
Collaborator
|
This pull request was successfully merged by @byCedric in 1bd9d36 When will my fix make it into a release? | How to file a pick request? |
jakex7
pushed a commit
to expo/react-native
that referenced
this pull request
Feb 4, 2025
… Android (facebook#48998) Summary: This is another attempt at fixing the Android HMR client for HTTPS proxied Metro instances. The previous one unintentionally [caused the following error](facebook#48970 (comment)): ``` java.lang.AssertionError: Method overloading is unsupported: com.facebook.react.devsupport.HMRClient#setup ``` This PR removes the overloading, and only adds the `scheme` property as a parameter to the existing `.setup` method. Aligning with the exact behavior we have on iOS. The alternative fix, which should NOT be backward breaking (if this is) - is to move this "infer the protocol from the bundle URL" to the JS side of the HMR client. Where we don't just always default to `http`, but instead default to `https IF port === 443, otherwise http`. It's a bit more hacky, but shouldn't cause any other issues. _**Ideally**_, we have the same working behavior on both Android and iOS without workarounds. <details><summary>Alternative workaround</summary> See [this change](facebook/react-native@main...byCedric:react-native:patch-2). <img width="1179" alt="image" src="https://github.com/user-attachments/assets/47c365bc-6df8-43e6-ad7d-5a667e350cd4" /> </details> See full explanation on facebook#48970 > We've noticed that the HMR on Android doesn't seem to be connecting when using a HTTPS-proxied Metro instance, where the proxy is hosted through Cloudflare. This is only an issue on Android - not iOS - and likely caused by the HMR Client not being set up properly on Android. > >- On Android, we run `.setup('android', <bundleEntryPath>, <proxiedMetroHost>, <proxiedMetroPort>, <hmrEnabled>)` in the [**react/devsupport/DevSupportManagerBase.java**](https://github.com/facebook/react-native/blob/53d94c3abe3fcd2168b512652bc0169956bffa39/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerBase.java#L689-L691) file. >- On iOS, we run `[self.callableJSModules invokeModule:@"HMRClient" method:@"setup" withArgs:@[ RCTPlatformName, path, host, RCTNullIfNil(port), @(isHotLoadingEnabled), scheme ]];` in the [**React/CoreModules /RCTDevSettings.mm**](https://github.com/facebook/react-native/blob/53d94c3abe3fcd2168b512652bc0169956bffa39/packages/react-native/React/CoreModules/RCTDevSettings.mm#L488-L491) file. > >Notice how Android does not pass in the scheme/protocol of the bundle URL, while iOS actually does? Unfortunately, because the default protocol (`http`) mismatches on Android when using HTTPS proxies, we actually try to connect the HMR client over `http` instead of `https` - while still using port 443 - which is rejected by Cloudflare's infrastructure even before we can redirect or mitigate this issue. And the rejection is valid, as we basically try to connect on `http://<host>:443` (the source URL is `https`, so the port is infered as `443`). > >This change adds scheme propagation to Android, exactly like we do on iOS for the HMR Client. [ANDROID] [FIXED] Pass the bundle URL protocol when setting up HMR client on Android <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#48998 Test Plan: See full explanation on facebook#48970 > It's a little bit hard to test this out yourself, since you'd need a HTTPS-based proxy and reject HTTP connections for HTTPS/WSS Websocket requests. > >You can set this up through: >- `bun create expo@latest ./test-app` >- `cd ./test-app` >- `touch .env` >- Set `EXPO_PACKAGER_PROXY_URL=https://<proxied-metro-hostname>` in **.env** >- Set `REACT_NATIVE_PACKAGER_HOSTNAME=<proxied-metro-hostname>` in **.env** >- `bun run start` > >Setting both these envvars, the bundle URL in the manifest is set to `https://...` - which triggers this HMR issue on Android. You can validate the **.env** setup through: > >```bash >curl "http://localhost:8081" -H "expo-platform: android" | jq .launchAsset.url >``` > >This should point the entry bundle URL towards the `EXPO_PACKAGER_PROXY_URL`. Reviewed By: cortinico Differential Revision: D68768351 Pulled By: javache fbshipit-source-id: 49bf1dc60f11b2af6e57177141270632d62ab564
jakex7
added a commit
to expo/expo
that referenced
this pull request
Feb 4, 2025
…roid (#34660) # Why Backport facebook/react-native#48998 to expo go app # Test Plan Expo Go builds correctly and clean app launches without issues.
gabrieldonadel
pushed a commit
to expo/react-native
that referenced
this pull request
Feb 6, 2025
… Android (facebook#48998) Summary: This is another attempt at fixing the Android HMR client for HTTPS proxied Metro instances. The previous one unintentionally [caused the following error](facebook#48970 (comment)): ``` java.lang.AssertionError: Method overloading is unsupported: com.facebook.react.devsupport.HMRClient#setup ``` This PR removes the overloading, and only adds the `scheme` property as a parameter to the existing `.setup` method. Aligning with the exact behavior we have on iOS. The alternative fix, which should NOT be backward breaking (if this is) - is to move this "infer the protocol from the bundle URL" to the JS side of the HMR client. Where we don't just always default to `http`, but instead default to `https IF port === 443, otherwise http`. It's a bit more hacky, but shouldn't cause any other issues. _**Ideally**_, we have the same working behavior on both Android and iOS without workarounds. <details><summary>Alternative workaround</summary> See [this change](facebook/react-native@main...byCedric:react-native:patch-2). <img width="1179" alt="image" src="https://github.com/user-attachments/assets/47c365bc-6df8-43e6-ad7d-5a667e350cd4" /> </details> See full explanation on facebook#48970 > We've noticed that the HMR on Android doesn't seem to be connecting when using a HTTPS-proxied Metro instance, where the proxy is hosted through Cloudflare. This is only an issue on Android - not iOS - and likely caused by the HMR Client not being set up properly on Android. > >- On Android, we run `.setup('android', <bundleEntryPath>, <proxiedMetroHost>, <proxiedMetroPort>, <hmrEnabled>)` in the [**react/devsupport/DevSupportManagerBase.java**](https://github.com/facebook/react-native/blob/53d94c3abe3fcd2168b512652bc0169956bffa39/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerBase.java#L689-L691) file. >- On iOS, we run `[self.callableJSModules invokeModule:@"HMRClient" method:@"setup" withArgs:@[ RCTPlatformName, path, host, RCTNullIfNil(port), @(isHotLoadingEnabled), scheme ]];` in the [**React/CoreModules /RCTDevSettings.mm**](https://github.com/facebook/react-native/blob/53d94c3abe3fcd2168b512652bc0169956bffa39/packages/react-native/React/CoreModules/RCTDevSettings.mm#L488-L491) file. > >Notice how Android does not pass in the scheme/protocol of the bundle URL, while iOS actually does? Unfortunately, because the default protocol (`http`) mismatches on Android when using HTTPS proxies, we actually try to connect the HMR client over `http` instead of `https` - while still using port 443 - which is rejected by Cloudflare's infrastructure even before we can redirect or mitigate this issue. And the rejection is valid, as we basically try to connect on `http://<host>:443` (the source URL is `https`, so the port is infered as `443`). > >This change adds scheme propagation to Android, exactly like we do on iOS for the HMR Client. [ANDROID] [FIXED] Pass the bundle URL protocol when setting up HMR client on Android <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#48998 Test Plan: See full explanation on facebook#48970 > It's a little bit hard to test this out yourself, since you'd need a HTTPS-based proxy and reject HTTP connections for HTTPS/WSS Websocket requests. > >You can set this up through: >- `bun create expo@latest ./test-app` >- `cd ./test-app` >- `touch .env` >- Set `EXPO_PACKAGER_PROXY_URL=https://<proxied-metro-hostname>` in **.env** >- Set `REACT_NATIVE_PACKAGER_HOSTNAME=<proxied-metro-hostname>` in **.env** >- `bun run start` > >Setting both these envvars, the bundle URL in the manifest is set to `https://...` - which triggers this HMR issue on Android. You can validate the **.env** setup through: > >```bash >curl "http://localhost:8081" -H "expo-platform: android" | jq .launchAsset.url >``` > >This should point the entry bundle URL towards the `EXPO_PACKAGER_PROXY_URL`. Reviewed By: cortinico Differential Revision: D68768351 Pulled By: javache fbshipit-source-id: 49bf1dc60f11b2af6e57177141270632d62ab564
cortinico
pushed a commit
that referenced
this pull request
Mar 11, 2025
… Android (#48998) Summary: This is another attempt at fixing the Android HMR client for HTTPS proxied Metro instances. The previous one unintentionally [caused the following error](#48970 (comment)): ``` java.lang.AssertionError: Method overloading is unsupported: com.facebook.react.devsupport.HMRClient#setup ``` This PR removes the overloading, and only adds the `scheme` property as a parameter to the existing `.setup` method. Aligning with the exact behavior we have on iOS. The alternative fix, which should NOT be backward breaking (if this is) - is to move this "infer the protocol from the bundle URL" to the JS side of the HMR client. Where we don't just always default to `http`, but instead default to `https IF port === 443, otherwise http`. It's a bit more hacky, but shouldn't cause any other issues. _**Ideally**_, we have the same working behavior on both Android and iOS without workarounds. <details><summary>Alternative workaround</summary> See [this change](main...byCedric:react-native:patch-2). <img width="1179" alt="image" src="https://github.com/user-attachments/assets/47c365bc-6df8-43e6-ad7d-5a667e350cd4" /> </details> See full explanation on #48970 > We've noticed that the HMR on Android doesn't seem to be connecting when using a HTTPS-proxied Metro instance, where the proxy is hosted through Cloudflare. This is only an issue on Android - not iOS - and likely caused by the HMR Client not being set up properly on Android. > >- On Android, we run `.setup('android', <bundleEntryPath>, <proxiedMetroHost>, <proxiedMetroPort>, <hmrEnabled>)` in the [**react/devsupport/DevSupportManagerBase.java**](https://github.com/facebook/react-native/blob/53d94c3abe3fcd2168b512652bc0169956bffa39/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevSupportManagerBase.java#L689-L691) file. >- On iOS, we run `[self.callableJSModules invokeModule:@"HMRClient" method:@"setup" withArgs:@[ RCTPlatformName, path, host, RCTNullIfNil(port), @(isHotLoadingEnabled), scheme ]];` in the [**React/CoreModules /RCTDevSettings.mm**](https://github.com/facebook/react-native/blob/53d94c3abe3fcd2168b512652bc0169956bffa39/packages/react-native/React/CoreModules/RCTDevSettings.mm#L488-L491) file. > >Notice how Android does not pass in the scheme/protocol of the bundle URL, while iOS actually does? Unfortunately, because the default protocol (`http`) mismatches on Android when using HTTPS proxies, we actually try to connect the HMR client over `http` instead of `https` - while still using port 443 - which is rejected by Cloudflare's infrastructure even before we can redirect or mitigate this issue. And the rejection is valid, as we basically try to connect on `http://<host>:443` (the source URL is `https`, so the port is infered as `443`). > >This change adds scheme propagation to Android, exactly like we do on iOS for the HMR Client. ## Changelog: [ANDROID] [FIXED] Pass the bundle URL protocol when setting up HMR client on Android <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: #48998 Test Plan: See full explanation on #48970 > It's a little bit hard to test this out yourself, since you'd need a HTTPS-based proxy and reject HTTP connections for HTTPS/WSS Websocket requests. > >You can set this up through: >- `bun create expo@latest ./test-app` >- `cd ./test-app` >- `touch .env` >- Set `EXPO_PACKAGER_PROXY_URL=https://<proxied-metro-hostname>` in **.env** >- Set `REACT_NATIVE_PACKAGER_HOSTNAME=<proxied-metro-hostname>` in **.env** >- `bun run start` > >Setting both these envvars, the bundle URL in the manifest is set to `https://...` - which triggers this HMR issue on Android. You can validate the **.env** setup through: > >```bash >curl "http://localhost:8081" -H "expo-platform: android" | jq .launchAsset.url >``` > >This should point the entry bundle URL towards the `EXPO_PACKAGER_PROXY_URL`. Reviewed By: cortinico Differential Revision: D68768351 Pulled By: javache fbshipit-source-id: 49bf1dc60f11b2af6e57177141270632d62ab564
Collaborator
|
This pull request was successfully merged by @byCedric in 68055f2 When will my fix make it into a release? | How to file a pick request? |
This was referenced Apr 1, 2025
This file contains hidden or 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
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:
This is another attempt at fixing the Android HMR client for HTTPS proxied Metro instances. The previous one unintentionally caused the following error:
This PR removes the overloading, and only adds the
schemeproperty as a parameter to the existing.setupmethod. Aligning with the exact behavior we have on iOS.The alternative fix, which should NOT be backward breaking (if this is) - is to move this "infer the protocol from the bundle URL" to the JS side of the HMR client. Where we don't just always default to
http, but instead default tohttps IF port === 443, otherwise http. It's a bit more hacky, but shouldn't cause any other issues. Ideally, we have the same working behavior on both Android and iOS without workarounds.Alternative workaround
See this change.
See full explanation on #48970
Changelog:
[ANDROID] [FIXED] Pass the bundle URL protocol when setting up HMR client on Android
Test Plan:
See full explanation on #48970