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

WIP: bump okhttp to 4.x #30694

Closed
wants to merge 4 commits into from
Closed

Conversation

dulmandakh
Copy link
Contributor

Summary

Bump OkHTTP to 4.x, which uses Kotlin. This

https://square.github.io/okhttp/upgrading_to_okhttp_4/

Changelog

[Android] [Changed] - Bump OkHTTP to 4.x,

Test Plan

@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. labels Jan 6, 2021
@react-native-bot react-native-bot added the Platform: Android Android applications. label Jan 6, 2021
@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: d3a3ce8

@dulmandakh dulmandakh changed the title bump okhttp to 4.x WIP: bump okhttp to 4.x Jan 6, 2021
@arazabishov
Copy link
Contributor

Thanks for doing his @dulmandakh. I've searched-up the repo for usages of internal OkHttp APIs and found one at RequestBodyUtil:L135. It is a call to Util.closeQuietly, which we can easily replicate. Do you think it is something that we can fix in this PR?

@dulmandakh
Copy link
Contributor Author

Any help is welcome. They say that they made some APIs final thus having problems with mocking.

@arazabishov
Copy link
Contributor

Alright, do you want me to push to this branch then? I will take a look at it later today

@arazabishov
Copy link
Contributor

Also, how do you usually test changes like this? (dep upgrades for example)

@dulmandakh
Copy link
Contributor Author

First of we need to make tests pass, but with this change test_android is failing. Please feel free work which ever way works for you

@ecreeth
Copy link
Contributor

ecreeth commented Jan 11, 2021

@dulmandakh I think that we can remove TLSSocketFactory

This class was needed for TLS 1.2 support on Android 4.x

@arazabishov
Copy link
Contributor

Hey folks. I've picked up this branch to fix broken tests and created a follow-up PR here: #31084. Feel free to review and leave comments if any.

@dulmandakh
Copy link
Contributor Author

closing in favor of #31084

@dulmandakh dulmandakh closed this Mar 13, 2021
@dulmandakh dulmandakh deleted the okhttp-4.x branch March 13, 2021 04:40
facebook-github-bot pushed a commit that referenced this pull request Apr 8, 2021
Summary:
Extends #30694 to fix tests.

OkHttp v4 was released almost a year ago. Even though v3 is still receiving security and bug fixes, most of the new improvements and features are landing in v4. This PR bumps OkHttp from v3 to v4 and addresses backward-incompatible changes.

Side effects of this upgrade:
 - OkHttp v4 depends on Kotlin's standard library, so react-native will have a transitive dependency on it.
 - The dex method count of test apk has exceeded the maximum, so multidexing had to be enabled for android tests.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Android] [Changed] - Bumping OkHttp from v3 to v4.

Pull Request resolved: #31084

Test Plan: Automated (relying on the test suite) and manual testing.

Reviewed By: fkgozali

Differential Revision: D27597430

Pulled By: ShikaSD

fbshipit-source-id: 967379b41c2bcd7cfd4083f65059f5da467b8a91
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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. Platform: Android Android applications.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants