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

Allow for changing the okhttpclientprovider builder #17198

Conversation

erlendfh
Copy link

@erlendfh erlendfh commented Dec 14, 2017

Motivation

This is another attempt at reviving OkHttpClientProvider as a way of controlling the OkHttpClients used by react native. See previous attempts #14068 and #14675, and the original change here: 0a71f48 .

Test Plan

Unit test

Release Notes

[ANDROID] [ENHANCEMENT] [Networking] - Added support for swapping out OkHttpClient.Builder in OkHttpClientProvider

@facebook-github-bot facebook-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 Dec 14, 2017
@pull-bot
Copy link

Warnings
⚠️

📋 Release Notes - This PR appears to be missing Release Notes.

@facebook-github-bot label Needs more information

Generated by 🚫 dangerJS

@facebook-github-bot
Copy link
Contributor

@erlendfh I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

@erlendfh
Copy link
Author

Maybe @hramos or @mkonicek could take a look at this okhttpclientprovider-related PR?

@greghe
Copy link
Contributor

greghe commented Jan 15, 2018

👍
issue #13487

@erlendfh
Copy link
Author

Any feedback or comments on this PR, @hramos or @mkonicek ? I'm open to alternative approaches, but since injecting new behavior into the networking code is possible on iOS it would be really useful to re-establish this possibility on Android - especially given that the current API gives you the impression that it's possible even though it isn't 😄

@hramos
Copy link
Contributor

hramos commented Feb 27, 2018

I don't have anything to add right now, other than there are some conflicts you might want to look at.

I'm not particularly familiar with this area of the codebase -- I'll let another core contributor jump in. Please ping me once this has been reviewed.

@erlendfh
Copy link
Author

erlendfh commented Mar 2, 2018

Seems this is already fixed in #17237 👍

@erlendfh erlendfh closed this Mar 2, 2018
@erlendfh erlendfh deleted the allow-for-changing-the-okhttpclientprovider-builder branch March 2, 2018 13:40
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants