react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Published 20 hours ago • verified publisher icon facebook

Remove okhttp internal util usage

Open adrianha opened this issue 2 years ago • 10 comments

Summary:

Remove okhttp internal Util usage so it can be compatible with okhttp 5.0.0 alpha.

Changelog:

[ANDROID] [CHANGED] - Remove okhttp3 internal util usage

Test Plan:

None, the implementation is based on okhttp internal util

adrianha avatar Jun 13 '23 10:06 adrianha

Do we know if this has the same behaviour as before, where checked exceptions are ignored? https://github.com/square/okhttp/blob/8c8c3dbcfa91e28de2e13975ec414e07f153fde4/okhttp/src/commonMain/kotlin/okhttp3/internal/-UtilCommon.kt#LL282C1-L288C2

javache avatar Jun 13 '23 13:06 javache

Source exception

just added the IOException catch block here, I think it should be enough to silence the exception as Source only throws IOException. Any thoughts @javache @cortinico?

adrianha avatar Jun 14 '23 03:06 adrianha

just added the IOException catch block here, I think it should be enough to silence the exception as Source only throws IOException. Any thoughts @javache @cortinico?

I would suggest you just copy over the same code as closeQuietly (I would also add a comment to where the code was vendored from).

You're right that close only fires IOException but, as users might swap different Java & OkHTTP version, the exception types could differ in the future.

cortinico avatar Jun 14 '23 15:06 cortinico

/rebase

cortinico avatar Jun 14 '23 15:06 cortinico

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,757,538 -146
android hermes armeabi-v7a 8,070,118 -140
android hermes x86 9,250,139 -142
android hermes x86_64 9,099,283 -135
android jsc arm64-v8a 9,318,912 +80
android jsc armeabi-v7a 8,508,818 +78
android jsc x86 9,382,411 +91
android jsc x86_64 9,635,644 +76

Base commit: 8c9e37561f56d50a9f7bbd6d67eeab3f85c3a66e Branch: main

analysis-bot avatar Jun 14 '23 16:06 analysis-bot

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

facebook-github-bot avatar Jun 15 '23 14:06 facebook-github-bot

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

facebook-github-bot avatar Jun 15 '23 15:06 facebook-github-bot

thank you for reviewing @cortinico, any actionables from my side?

adrianha avatar Jun 15 '23 15:06 adrianha

Not for now, we'll be merging this internally. It can take up to a couple of days

cortinico avatar Jun 15 '23 15:06 cortinico

great, thanks!

adrianha avatar Jun 15 '23 16:06 adrianha

@cortinico merged this pull request in facebook/react-native@3e3032636dc90a21a499492dcb88f819bcf4f003.

facebook-github-bot avatar Jun 15 '23 23:06 facebook-github-bot