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

Fix port as -1 if dev server without specifying port on Android #34705

Closed
wants to merge 4 commits into from

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Sep 16, 2022

Summary

when specifying dev server without port, e.g. http://www.example.com/, there are some issues.

  1. redbox error

  1. showing -1 in loading view

the root cause is coming from java.net.URL.getPort() will return -1 when the url doesn't have a port.

this pr replaces the parser to okhttp3.HttpUrl that it will have default port 80 for http or port 443 for https. the two call paths should only serve http/https address, not file:// address. it should be safe to change from java.net.URL to okhttp3.HttpUrl.
Update: adding java.net.URL.getDefaultPort fallback when the default port is -1

not fully related, in the case above, android will connect to ws://www.example.com/:8097 for react-devtools
we should strip the trailing slash in setUpReactDevTools.js

Changelog

[Android] [Fixed] - Fix port as -1 if dev server without specifying port on Android

Test Plan

test on rn-tester with the following steps

  1. yarn start
  2. open another terminal and run ngrok http 8081 and it will return a tunnel url, e.g. 71a1-114-36-194-97.jp.ngrok.io
  3. open dev setting in app and change the dev server to 71a1-114-36-194-97.jp.ngrok.io
  4. reload the app

@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. Contributor A React Native contributor. p: Expo Partner: Expo Partner labels Sep 16, 2022
@react-native-bot react-native-bot added Bug Platform: Android Android applications. labels Sep 16, 2022
@Kudo Kudo marked this pull request as ready for review Sep 16, 2022
@facebook-github-bot facebook-github-bot added the Shared with React Native Team Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 16, 2022
Kudo added a commit to expo/expo that referenced this pull request Sep 16, 2022
# Why

when load js bundle from a URL without an explicit port, e.g. http://www.example.com/ , the app will crash.
the root cause is that `android.net.Uri.getPort()` will return -1 when there's no port.

related facebook/react-native#34705 and ENG-5932

# How

use `okhttp3.HttpUrl.defaultPort(scheme)` as fallback when port is -1.

since the parameter in `injectReactInterceptor` is already android.net.Uri, i don't want to use HttpUrl to parse it again. only change the default port would be lower risk.

# Test Plan

test on bare-expo
1. [enable dev-launcher](https://github.com/expo/expo/blob/7331e13192c5b798ec67ec180949e1ef4e4da228/apps/bare-expo/android/app/src/main/java/dev/expo/payments/MainApplication.java#L25)
2. `EXPO_NO_DEFAULT_PORT=1 npx expo start --tunnel`
3. in dev-launcher, load the bundle url, e.g. `exp://nquwmbi.kudochien.19000.exp.direct`
lukmccall pushed a commit to expo/expo that referenced this pull request Sep 16, 2022
# Why

when load js bundle from a URL without an explicit port, e.g. http://www.example.com/ , the app will crash.
the root cause is that `android.net.Uri.getPort()` will return -1 when there's no port.

related facebook/react-native#34705 and ENG-5932

# How

use `okhttp3.HttpUrl.defaultPort(scheme)` as fallback when port is -1.

since the parameter in `injectReactInterceptor` is already android.net.Uri, i don't want to use HttpUrl to parse it again. only change the default port would be lower risk.

# Test Plan

test on bare-expo
1. [enable dev-launcher](https://github.com/expo/expo/blob/7331e13192c5b798ec67ec180949e1ef4e4da228/apps/bare-expo/android/app/src/main/java/dev/expo/payments/MainApplication.java#L25)
2. `EXPO_NO_DEFAULT_PORT=1 npx expo start --tunnel`
3. in dev-launcher, load the bundle url, e.g. `exp://nquwmbi.kudochien.19000.exp.direct`
@facebook-github-bot
Copy link
Contributor

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

@cortinico
Copy link
Contributor

Hey @Kudo, sadly this PR is failing internally for some of our apps with:

error: no suitable method found for get(java.lang.String)
       HttpUrl sourceUrl = HttpUrl.get(getSourceUrl());
[CONTEXT]         HttpUrl sourceUrl = HttpUrl.get(getSourceUrl());
[CONTEXT]                                    ^
[CONTEXT]     method okhttp3.HttpUrl.get(java.net.URL) is not applicable
[CONTEXT]       (argument mismatch; java.lang.String cannot be converted to java.net.URL)
[CONTEXT]     method okhttp3.HttpUrl.get(java.net.URI) is not applicable
[CONTEXT]       (argument mismatch; java.lang.String cannot be converted to java.net.URI)
/data/sandcastle/boxes/eden-trunk-hg-fb4a-fbsource/xplat/js/react-native-github/ReactAndroid/src/main/java/com/facebook/react/devsupport/DevLoadingViewController.java:65: error: no suitable method found for get(java.lang.String)

I believe that HttpUrl.get(String) was added in a recent version of OkHTTP (need to verify). Could we update this to be backward compat using either URL or URI?

@Kudo
Copy link
Contributor Author

Kudo commented Sep 21, 2022

thanks @cortinico! any chances for you to update okhttp3 internally? i think the version is older than okhttp 3.11.0 and that was four years ago.

i don't want to convert String -> {URL,Uri} -> HttpUrl parse (that would call {URL,Uri}.toString() internally).
the other idea is to use HttpUrl.parse() than HttpUrl.get(), but it just returns null without exception error context from illegal url. that's why i am asking the feasibility to upgrade okhttp3 and use the better `HttpUrl.get().

@Kudo
Copy link
Contributor Author

Kudo commented Sep 22, 2022

any chances for you to update okhttp3 internally? i think the version is older than okhttp 3.11.0 and that was four years ago.

i find there's java.net.URL.getDefaultPort. i've updated the pr by using it. that would also simplify the change.

@cortinico
Copy link
Contributor

i find there's java.net.URL.getDefaultPort. i've updated the pr by using it. that would also simplify the change.

That's great! I could look into updating OkHTTP but the failure is happening in 3/4 different apps and it could take me some time.

I'll re-import your change and give it a go 👍

@facebook-github-bot
Copy link
Contributor

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @Kudo in 3d7e138.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Sep 22, 2022
@Kudo
Copy link
Contributor Author

Kudo commented Sep 22, 2022

thanks @cortinico for landing this 🚀

@Kudo Kudo deleted the android-dev-server-port branch Sep 22, 2022
Ddv0623 pushed a commit to preciofishbone/expo that referenced this pull request Sep 26, 2022
# Why

when load js bundle from a URL without an explicit port, e.g. http://www.example.com/ , the app will crash.
the root cause is that `android.net.Uri.getPort()` will return -1 when there's no port.

related facebook/react-native#34705 and ENG-5932

# How

use `okhttp3.HttpUrl.defaultPort(scheme)` as fallback when port is -1.

since the parameter in `injectReactInterceptor` is already android.net.Uri, i don't want to use HttpUrl to parse it again. only change the default port would be lower risk.

# Test Plan

test on bare-expo
1. [enable dev-launcher](https://github.com/expo/expo/blob/7331e13192c5b798ec67ec180949e1ef4e4da228/apps/bare-expo/android/app/src/main/java/dev/expo/payments/MainApplication.java#L25)
2. `EXPO_NO_DEFAULT_PORT=1 npx expo start --tunnel`
3. in dev-launcher, load the bundle url, e.g. `exp://nquwmbi.kudochien.19000.exp.direct`
kelset pushed a commit that referenced this pull request Oct 3, 2022
Summary:
when specifying dev server without port, e.g. http://www.example.com/, there are some issues.

1. redbox error
<img src="https://user-images.githubusercontent.com/46429/190540390-8ee420f2-7642-427b-9f2e-e0c6d31015f8.png" width="30%">

2. showing -1 in loading view

<img src="https://user-images.githubusercontent.com/46429/190540727-158f35ad-359f-443a-a4b0-768dd2f7e400.png" width="50%">

the root cause is coming from [`java.net.URL.getPort()` will return -1 when the url doesn't have a port](https://developer.android.com/reference/java/net/URL#getPort()). this pr replaces the parser to [`okhttp3.HttpUrl`](https://square.github.io/okhttp/4.x/okhttp/okhttp3/-http-url/#port) that it will have default port 80 for http or port 443 for https. the two call paths should only serve http/https address, not file:// address. it should be safe to change from java.net.URL to okhttp3.HttpUrl.

not fully related, in the case above, android will connect to `ws://www.example.com/:8097` for react-devtools
we should strip the trailing slash in *setUpReactDevTools.js*

## Changelog

[Android] [Fixed] - Fix port as -1 if dev server without specifying port on Android

Pull Request resolved: #34705

Test Plan:
test on rn-tester with the following steps

1. `yarn start`
2. open another terminal and run `ngrok http 8081` and it will return a tunnel url, e.g. `71a1-114-36-194-97.jp.ngrok.io`
3. open dev setting in app and change the dev server to `71a1-114-36-194-97.jp.ngrok.io`
5. reload the app

Reviewed By: cipolleschi

Differential Revision: D39573988

Pulled By: cortinico

fbshipit-source-id: 397df90ab30533207bd87a3f069132d97c22c7fd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 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. Merged This PR has been merged. p: Expo Partner: Expo Partner Platform: Android Android applications. Shared with React Native Team 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.

None yet

4 participants