Skip to content

Remove okhttp internal util usage - #37843

Closed
adrianha wants to merge 3 commits into
react:mainfrom
adrianha:remove-okhttp-internal
Closed

Remove okhttp internal util usage#37843
adrianha wants to merge 3 commits into
react:mainfrom
adrianha:remove-okhttp-internal

Conversation

@adrianha

@adrianhaadrianha commented Jun 13, 2023•
edited
Loading

Copy link
Copy Markdown
Contributor

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

@facebook-github-botfacebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jun 13, 2023
@javache

javache commented Jun 13, 2023

Copy link
Copy Markdown
Contributor

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

cortinico
cortinico requested changes Jun 13, 2023
View reviewed changes
Comment thread...ct-native/ReactAndroid/src/main/java/com/facebook/react/modules/network/RequestBodyUtil.java Outdated

@cortinicocortinicoJun 13, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we copy over the code of closeQuitely, namely:

fun Closeable.closeQuietly() {
try {
close()
} catch (rethrown:RuntimeException) {
throw rethrown
} catch (_:Exception) {
}
}

@adrianhaadrianhaJun 13, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

thanks for the suggestion, i’ll update it later

@adrianha

adrianha commented Jun 14, 2023

Copy link
Copy Markdown
ContributorAuthor
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?

@cortinico

cortinico commented Jun 14, 2023

Copy link
Copy Markdown
Contributor

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.

@adrianha
adrianha requested a review from cortinicoJune 14, 2023 15:52
@cortinico

cortinico commented Jun 14, 2023

Copy link
Copy Markdown
Contributor

/rebase

@github-actions
github-actionsBotforce-pushed the remove-okhttp-internal branch from 135f5c8 to 7730b9dCompareJune 14, 2023 15:57
@analysis-bot

analysis-bot commented Jun 14, 2023•
edited
Loading

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,757,538-146
androidhermesarmeabi-v7a8,070,118-140
androidhermesx869,250,139-142
androidhermesx86_649,099,283-135
androidjscarm64-v8a9,318,912+80
androidjscarmeabi-v7a8,508,818+78
androidjscx869,382,411+91
androidjscx86_649,635,644+76

Base commit: 8c9e375
Branch: main

cortinico
cortinico requested changes Jun 14, 2023
View reviewed changes

@cortinicocortinico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

See my previous comment

@adrianha
adrianha requested a review from cortinicoJune 15, 2023 03:35
@facebook-github-bot

facebook-github-bot commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

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

1 similar comment
@facebook-github-bot

facebook-github-bot commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

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

@adrianha

adrianha commented Jun 15, 2023

Copy link
Copy Markdown
ContributorAuthor

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

@cortinico

cortinico commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

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

@adrianha

adrianha commented Jun 15, 2023

Copy link
Copy Markdown
ContributorAuthor

great, thanks!

@facebook-github-bot

facebook-github-bot commented Jun 15, 2023

Copy link
Copy Markdown
Contributor

@cortinico merged this pull request in 3e30326.

@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label Jun 15, 2023
adamaveray added a commit to adamaveray/react-native that referenced this pull request Jun 16, 2023
@adamaveray
Merge branch 'main' into feature/automaticKeyboardInsetsImprovements
9d7205f
* main: (135 commits)
translation auto-update for i18n/twilight.config.json on master
Interop: Introduce Bridge proxy
Remove okhttp internal util usage (react#37843)
Update debian to fix CI while updating Node (react#37841)
fix: foreground ripple crash on api < 23 (react#37901)
Re-add the top level LICENSE file (react#37916)
Deploy 0.209.0 to xplat (react#37921)
Re-enable direct debugging with JSC on iOS 16.4+ (react#37914)
add emitObjectProp in parser primitives (react#37904)
Make React-utils its own pod (react#37659)
feat: allow custom assignment of rootView to rootViewController (react#37873)
Switch xplat prettier config to hermes plugin (react#37915)
Set iOS AppState to inactive when app is launching (react#37690)
Use `fileExists` in replace_hermes script (react#37911)
(docs): fix license url (react#37909)
Revert D46719890: Re-enable direct debugging with JSC on iOS 16.4+
Re-enable direct debugging with JSC on iOS 16.4+ (react#37874)
Fix component type references in xplat (react#37903)
Remove usage of passthroughAnimatedPropExplicitValues in ScrollViewStickyHeader (react#37867)
test runtime lifecycle callback (react#37897)
...
@adrianha
adrianha deleted the remove-okhttp-internal branch June 29, 2023 23:42
kelset pushed a commit that referenced this pull request Jul 10, 2023
@adrianha@kelset
Remove okhttp internal util usage (#37843)
03b9b52
Summary:
Remove okhttp internal `Util` usage so it can be compatible with [okhttp 5.0.0 alpha](https://square.github.io/okhttp/changelogs/changelog/#version-500-alpha7).
## Changelog:
<!-- Help reviewers and the release process by writing your own changelog entry.
Pick one each for the category and type tags:
For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[ANDROID] [CHANGED] - Remove okhttp3 internal util usage
Pull Request resolved: #37843
Test Plan: None, the implementation is based on okhttp internal util
Reviewed By: NickGerleman
Differential Revision: D46764363
Pulled By: cortinico
fbshipit-source-id: e46770625f19057fa7374be15e92903d7966f880
Kudo pushed a commit to expo/react-native that referenced this pull request Jul 11, 2023
@adrianha@Kudo
Remove okhttp internal util usage (react#37843)
e2ac2da
Summary:
Remove okhttp internal `Util` usage so it can be compatible with [okhttp 5.0.0 alpha](https://square.github.io/okhttp/changelogs/changelog/#version-500-alpha7).
## Changelog:
<!-- Help reviewers and the release process by writing your own changelog entry.
Pick one each for the category and type tags:
For more details, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[ANDROID] [CHANGED] - Remove okhttp3 internal util usage
Pull Request resolved: react#37843
Test Plan: None, the implementation is based on okhttp internal util
Reviewed By: NickGerleman
Differential Revision: D46764363
Pulled By: cortinico
fbshipit-source-id: e46770625f19057fa7374be15e92903d7966f880
This was referenced Aug 1, 2023
Open
Open
This was referenced Aug 10, 2023
Closed
Merged
@cipolleschicipolleschi mentioned this pull request Sep 21, 2023
Merged
@cipolleschicipolleschi mentioned this pull request Oct 12, 2023
Merged
@cipolleschicipolleschi mentioned this pull request Nov 1, 2023
Merged
@stevenpetrykstevenpetryk mentioned this pull request Nov 28, 2023
Merged
@cipolleschicipolleschi mentioned this pull request Dec 27, 2023
Merged
This was referenced Jan 4, 2024
Merged
Merged
Merged
Closed
This was referenced Feb 28, 2024
Merged
Merged
Merged
@cipolleschicipolleschi mentioned this pull request Jun 28, 2024
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cortinicocortinicocortinico approved these changes

Assignees

No one assigned

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.MergedThis PR has been merged.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@adrianha@javache@cortinico@analysis-bot@facebook-github-bot