Inject configured OkHttpClient to networking module using MainPackageConfig #14068

thotegowda · 2017-05-19T18:43:28Z

Motivation

The main purpose of this PR is to enable brownfield applications to inject existingOkHttpClient instance to react-native networking sub system. Most native applications would have their OkHttpClient instance configured with headers, interceptors, logging, fallback handling, error handling, etc. It doesn't make much sense to duplicate all of this to use fetch in javascript. Currently the only way you could use existing OkHttpClient is by custom-bridge to native, but this will stops app from using fetch interface in JS.

With this commit, App can pass OkHttpClient instance to networking module using MainPackageConfig, while creating MainReactPackage.

Test Plan

Unit tests.

amilcar-andrade · 2017-05-19T19:27:14Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

+  }
+
+  private OkHttpClient createOkHttpClient(
+    OkHttpClientProvider okHttpClientProvider,


You can add the @Nullable annotation just like List<NetworkInterceptorCreator>

Yes good idea - makes the API clearer to users.

amilcar-andrade · 2017-05-19T19:28:45Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

  }

  /**
   * @param context the ReactContext of the application
   * @param defaultUserAgent the User-Agent header that will be set for all requests where the
   * caller does not provide one explicitly
-   * @param client the {@link OkHttpClient} to be used for networking
+   * @param okHttpClientProvider the {@link OkHttpClient} provider to be used for networking


You could update the {@link } to point to the OkHttpClientProvider

Also, probably you should just call it provider

I agree with both. Maybe httpClientProvider.

amilcar-andrade · 2017-05-19T19:30:00Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

+  }
+
+  /**
+   * @param context the ReactContext of the application


You could use the {@link} annotation for the java docs

nit @param context the {@link ReactApplicationContext} of the application

amilcar-andrade · 2017-05-19T19:31:25Z

ReactAndroid/src/main/java/com/facebook/react/shell/MainPackageConfig.java


    public Builder setFrescoConfig(ImagePipelineConfig frescoConfig) {
      mFrescoConfig = frescoConfig;
      return this;
    }

+    /**
+     * Ability to inject configured OkHttpClient provider to react-native networking subsystem.


Same comment about the {@link} annotation. I also dont think that the sentence This will be very useful is needed.

amilcar-andrade · 2017-05-19T19:31:41Z

ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java

@@ -77,6 +78,7 @@
  @Test
  public void testGetWithoutHeaders() throws Exception {
    OkHttpClient httpClient = mock(OkHttpClient.class);
+


amilcar-andrade · 2017-05-19T19:32:05Z

ReactAndroid/src/test/java/com/facebook/react/modules/network/NetworkingModuleTest.java

@@ -87,8 +89,10 @@ public Object answer(InvocationOnMock invocation) throws Throwable {
    OkHttpClient.Builder clientBuilder = mock(OkHttpClient.Builder.class);
    when(clientBuilder.build()).thenReturn(httpClient);
    when(httpClient.newBuilder()).thenReturn(clientBuilder);
+    OkHttpClientProvider clientProvider = mock(OkHttpClientProvider.class);


Same comment than before you can just call it provider

amilcar-andrade · 2017-05-19T19:33:46Z

@thotegowda I added some minor comments. By the way, I am not an official reviewer but I am going to do something similar to what you are trying to achieve.

mkonicek · 2017-05-22T19:36:47Z

ReactAndroid/src/main/java/com/facebook/react/modules/fresco/FrescoModule.java

@@ -154,7 +154,7 @@ private static ImagePipelineConfig getDefaultConfig(ReactContext context) {
    HashSet<RequestListener> requestListeners = new HashSet<>();
    requestListeners.add(new SystraceRequestListener());

-    OkHttpClient client = OkHttpClientProvider.createClient();
+    OkHttpClient client = new DefaultOkHttpProvider().get();


If fetching images should not be affected by the HTTP provider from MainPackageConfig we should document the MainPackageConfig only affects the fetch API.

It's probably easier, however, to just use the HTTP client configured in MainPackageConfig also here (for fetching images) to avoid confusion.

I haven't looked in to the details of how Fresco is using this client. But I guess, Fresco might need its own customised client configuration.

For now, I will add a comment in MainPackageConfig. Will take a look at this later. Would be great if someone with Fresco context can share some light.

OK sounds good, someone else can do that separately and we can keep this pull request simpler.

mkonicek · 2017-05-22T19:38:45Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/DefaultOkHttpProvider.java

+    On Android 4.1-4.4 (API level 16 to 19) TLS 1.1 and 1.2 are
+    available but not enabled by default. The following method
+    enables it.
+   */


Just a stylistic thing but I could you make this into a Javadoc please?

Also the method should be private - it's not used anywhere outside this class. I see it was public and just moved - so it was wrong previously but now is a good opportunity to make it private.

mkonicek · 2017-05-22T19:40:56Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/DefaultOkHttpProvider.java

+        specs.add(ConnectionSpec.CLEARTEXT);
+
+        client.connectionSpecs(specs);
+      } catch (Exception exc) {


nit: Please name this variable e - another opportunity to fix existing code a bit.

mkonicek · 2017-05-22T19:43:31Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/DefaultOkHttpProvider.java

+
+        client.connectionSpecs(specs);
+      } catch (Exception exc) {
+        FLog.e("OkHttpClientProvider", "Error while enabling TLS 1.2", exc);


This should be Flog.e(ReactConstants.TAG, "Error while enabling TLS 1.2", e); so it's easy to see all React Native related messages in logcat.

Again I realise this is just moved code but good opportunity for minor fixes.

mkonicek · 2017-05-22T19:43:46Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/DefaultOkHttpProvider.java

+        FLog.e("OkHttpClientProvider", "Error while enabling TLS 1.2", exc);
+      }
+    }
+


nit: Empty line not really needed.

mkonicek · 2017-05-22T19:46:43Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/OkHttpClientProvider.java


 /**
- * Helper class that provides the same OkHttpClient instance that will be used for all networking
- * requests.
+ *  Interface that provides the OkHttpClient instance, that will be used for all networking requests.


minor nit: No need for comma before "that".

mkonicek · 2017-05-22T19:47:22Z

ReactAndroid/src/main/java/com/facebook/react/shell/MainPackageConfig.java


 /**
 * Configuration for {@link MainReactPackage}
 */
 public class MainPackageConfig {

-  private ImagePipelineConfig mFrescoConfig;
+
+  private final OkHttpClientProvider mOkHttpClientProvider;


IMHO mHttpClientProvider would be as clear as it's shorter.

Should be @Nullable.

mkonicek · 2017-05-22T19:48:29Z

ReactAndroid/src/test/java/com/facebook/react/shell/MainPackageConfigTest.java

+    assertEquals(config.getOkHttpClientProvider().getClass(), DefaultOkHttpProvider.class);
+  }
+
+}


Awesome, thanks for adding this! 💯

mkonicek · 2017-05-22T19:49:20Z

Thanks for doing this! I get the motivation for doing this and think the PR makes a lot of sense. Left some quite minor comments and once you address them we can merge.

thotegowda · 2017-05-24T15:01:24Z

@mkonicek @amilcar-andrade Incorporated your feedback points. Please check.

amilcar-andrade · 2017-05-24T15:41:50Z

ReactAndroid/src/main/java/com/facebook/react/modules/network/NetworkingModule.java

+
+  /**
+   * @param context the ReactContext of the application
+   * @param httpClientProvider  {@link OkHttpClientProvider} to provide {@link OkHttpClient}


nit extra space httpClientProvider {

amilcar-andrade · 2017-05-24T15:42:47Z

@thotegowda cool. I think it looks good. But again I cannot merge or approve your PR. Thanks for doing this it will help a lot of us.

mkonicek · 2017-05-30T11:33:28Z

Thanks for addressing the comments! I don't have push access anymore but I just messaged contributors asking to merge this.

amilcar-andrade · 2017-05-30T14:19:04Z

@thotegowda you should probably squash your commits and rebase against master.

thotegowda · 2017-05-31T14:09:46Z

@amilcar-andrade - Done squashing commits.
@mkonicek - Thanks a lot for taking interest in this.

Knight704 · 2017-07-29T08:34:12Z

Sorry to comment here, but is there any way to customize okhttpclient currently without this PR? Just checked that OkHttpClientProvider has mechanics for this (like replaceOkHttpClient), but it seems that this injection isn't used internally in networking module

thotegowda · 2017-07-29T08:41:23Z

@Knight704 I had used replaceOkHttpClient before. But then one of the recent commit broke it. check commit and discussion here

Knight704 · 2017-07-29T09:49:32Z

@thotegowda, thanks, got it. So, if I understood correctly, now there is no way to do that, sad 👎

Knight704 · 2017-07-31T10:21:36Z

Okay, I came up to the workaround for this problem for now:

Extend MainReactPackage
Override method that return native modules and replace Networking provider with custom one,
In this custom provider return default NetworkingModule which has constructor that accept OkHttpClient. (But this constructor is package private and you have to put this custom provider inside facebook package to relax it's access).

tomoima525 · 2017-08-31T18:33:54Z

@hramos Hi, what is the status for this PR?

hramos · 2017-08-31T19:17:16Z

No news to share. It's on someone's queue internally, waiting for review. As it happens, merging this would require making additional changes to other apps in our internal code repository, which introduces a high level of friction that may keep this PR from getting handled any time soon.

tomoima525 · 2017-09-10T06:44:03Z

@hramos I see. So there is no good way to use Stetho which requires intercepting OkHttp Header

codebymikey · 2017-09-23T13:01:47Z

I'm really surprised this PR hasn't been merged in yet.
Anyways @tomoima525, I was able to workaround the issue a couple of months back using a variant of @Knight704 approach.

Here is a gist of it. Hope it's helpful.

Also @hramos, surely this is a relatively easy fix with almost no chance of breaking the existing tests/logic. Thoughts?

facebook-github-bot · 2017-10-04T17:04:11Z

@thotegowda 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.

leon087 · 2017-11-28T08:59:00Z

@hramos hi,what about this PR? #14675

AvivRubys · 2017-12-18T16:50:21Z

Any updates on this PR? I'd love to help if I can, but I'm not even sure what is this waiting for.

hey99xx · 2018-01-09T07:44:50Z

Any updates on this @hramos ?

If FB will not pull this in, can someone at least give an informal ship-it that confirms that the final code that will be merged will look like the current state of the PR. That way companies with brown-field apps that already compile RN in-house can manually make the same change and patch existing versions of RN between 0.43 and 0.52.

It's on someone's queue internally, waiting for review.

Also how long is this internal queue? It's been 4.5 months since that comment :S :(

Summary: Prior to 0a71f48, users could customise the OkHttp client used by React Native on Android by calling replaceOkHttpClient in OkHttpClientProvider. This functionality has a variety of legitimate applications from changing connection timeouts or pool size to Stetho integration. The challenge is to add back support for replacing the client without causing a breaking change or reintroducing the problems olegbl sought to address in his original commit. Introducing a client factory archives these aims, it adds a new, backwards compatible interface and is called each time a client is requested rather than re-using the same instance (unless you explicitly want this behaviour, in which case you could replicate it using a static class property inside your custom factory). A number of PRs have been opened to add this functionality: #14675, #14068. I don't have a lot of Java experience so I'm open to better/more idiomatic ways to achieve this :) Create React Native application and set a custom factory in the constructor, e.g. `OkHttpClientProvider.setOkHttpClientFactory(new CustomNetworkModule());` Where a custom factory would look like: ``` class CustomNetworkModule implements OkHttpClientFactory { public OkHttpClient createNewNetworkModuleClient() { return new OkHttpClient.Builder().build(); } } ``` Remove the existing replace client method to prevent accident use and alert existing users that its functionality has changed: #16972 [Android] [Minor] [Networking] - | Provide interface for customising the OkHttp client used by React Native | Closes #17237 Differential Revision: D6837734 Pulled By: hramos fbshipit-source-id: 81e63df7716e6f9039ea12e99233f6336c6dd7ef

hey99xx · 2018-02-01T02:09:46Z

Any updates?

Edit: If #17237 will be the preferred solution, we can actually close this now. I am ok with either pull-request, and the other one looks to be less invasive to me.

Summary: Prior to 0a71f48, users could customise the OkHttp client used by React Native on Android by calling replaceOkHttpClient in OkHttpClientProvider. This functionality has a variety of legitimate applications from changing connection timeouts or pool size to Stetho integration. The challenge is to add back support for replacing the client without causing a breaking change or reintroducing the problems olegbl sought to address in his original commit. Introducing a client factory archives these aims, it adds a new, backwards compatible interface and is called each time a client is requested rather than re-using the same instance (unless you explicitly want this behaviour, in which case you could replicate it using a static class property inside your custom factory). A number of PRs have been opened to add this functionality: facebook#14675, facebook#14068. I don't have a lot of Java experience so I'm open to better/more idiomatic ways to achieve this :) Create React Native application and set a custom factory in the constructor, e.g. `OkHttpClientProvider.setOkHttpClientFactory(new CustomNetworkModule());` Where a custom factory would look like: ``` class CustomNetworkModule implements OkHttpClientFactory { public OkHttpClient createNewNetworkModuleClient() { return new OkHttpClient.Builder().build(); } } ``` Remove the existing replace client method to prevent accident use and alert existing users that its functionality has changed: facebook#16972 [Android] [Minor] [Networking] - | Provide interface for customising the OkHttp client used by React Native | Closes facebook#17237 Differential Revision: D6837734 Pulled By: hramos fbshipit-source-id: 81e63df7716e6f9039ea12e99233f6336c6dd7ef

hey99xx · 2018-06-12T18:41:07Z

@hramos Given that 22efd95 got released with 0.54, this PR is no longer necessary. They were achieving the same thing through different implementations, but it's hard to keep them both. Can we close this issue?

Summary: This method was originally intended to replace the OkHttp client used by React Native's networking library. However it has effectively been a noop since 0a71f48#diff-177100ae5a977e4060b54cc2b34c79a7, when the Networking library was modified to create a new client rather than use the reference provided by OkHttpClientProvider. Leaving this code in place is dangerous. There is no indication to users upgrading React Native that the method is no longer replacing the OkHttpClient used by the Networking library. Any functionality reliant on overriding the client will silently break. This caused us some problems internally. There's been a PR out for some time that seeks to reintroduce this functionality: #14068 I've also put up a new PR that adds an interface for replacing the client without introducing breaking changes: #17237 Do our unit tests continue to pass? Should be safe as this method is not used anywhere inside React Native. [ANDROID] [BREAKING] [Networking] - removed replaceOkHttpClient method in OkHttpClientProvider. Pull Request resolved: #16972 Differential Revision: D13838805 Pulled By: cpojer fbshipit-source-id: 43606d1d141afb9b5dda4dd64e5ac5448771b45c

Summary: This method was originally intended to replace the OkHttp client used by React Native's networking library. However it has effectively been a noop since facebook@0a71f48#diff-177100ae5a977e4060b54cc2b34c79a7, when the Networking library was modified to create a new client rather than use the reference provided by OkHttpClientProvider. Leaving this code in place is dangerous. There is no indication to users upgrading React Native that the method is no longer replacing the OkHttpClient used by the Networking library. Any functionality reliant on overriding the client will silently break. This caused us some problems internally. There's been a PR out for some time that seeks to reintroduce this functionality: facebook#14068 I've also put up a new PR that adds an interface for replacing the client without introducing breaking changes: facebook#17237 Do our unit tests continue to pass? Should be safe as this method is not used anywhere inside React Native. [ANDROID] [BREAKING] [Networking] - removed replaceOkHttpClient method in OkHttpClientProvider. Pull Request resolved: facebook#16972 Differential Revision: D13838805 Pulled By: cpojer fbshipit-source-id: 43606d1d141afb9b5dda4dd64e5ac5448771b45c

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. GH Review: review-needed labels May 19, 2017

thotegowda referenced this pull request in thotegowda/react-native May 19, 2017

first cut

b9aaf77

amilcar-andrade reviewed May 19, 2017

View reviewed changes

mkonicek reviewed May 22, 2017

View reviewed changes

amilcar-andrade reviewed May 24, 2017

View reviewed changes

shergin added the Android label May 26, 2017

thotegowda force-pushed the master branch from 59137c3 to 71217d1 Compare May 31, 2017 14:06

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels May 31, 2017

andon mentioned this pull request Nov 18, 2017

Networking on Android doesn't use Cache #16795

Closed

cdlewis mentioned this pull request Nov 24, 2017

Remove replaceOkHttpClient method #16972

Closed

erlendfh mentioned this pull request Dec 14, 2017

Allow for changing the okhttpclientprovider builder #17198

Closed

omerlh mentioned this pull request Dec 18, 2017

Integration with React Native is broken datatheorem/TrustKit-Android#26

Closed

cdlewis mentioned this pull request Jan 15, 2018

Add back ability to customise OkHttp client #17237

Closed

hramos removed the GH Review: accepted label Mar 8, 2018

react-native-bot added Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Ran Commands One of our bots successfully processed a command. labels Mar 16, 2018

janicduplessis removed the Android 🤖 label Mar 19, 2018

react-native-bot added the ✅Test Plan label May 15, 2018

hramos closed this Jun 12, 2018

Inject configured OkHttpClient to networking module using MainPackageConfig #14068

Inject configured OkHttpClient to networking module using MainPackageConfig #14068

Conversation

thotegowda commented May 19, 2017 • edited Loading

Motivation

Test Plan

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkonicek May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amilcar-andrade commented May 19, 2017

mkonicek May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkonicek Jun 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkonicek May 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkonicek commented May 22, 2017 • edited Loading

thotegowda commented May 24, 2017

Choose a reason for hiding this comment

amilcar-andrade commented May 24, 2017

mkonicek commented May 30, 2017 • edited Loading

amilcar-andrade commented May 30, 2017

thotegowda commented May 31, 2017

Knight704 commented Jul 29, 2017

thotegowda commented Jul 29, 2017

Knight704 commented Jul 29, 2017

Knight704 commented Jul 31, 2017

tomoima525 commented Aug 31, 2017

hramos commented Aug 31, 2017

tomoima525 commented Sep 10, 2017

codebymikey commented Sep 23, 2017

facebook-github-bot commented Oct 4, 2017

leon087 commented Nov 28, 2017

AvivRubys commented Dec 18, 2017

hey99xx commented Jan 9, 2018 • edited Loading

hey99xx commented Feb 1, 2018 • edited Loading

hey99xx commented Jun 12, 2018

thotegowda commented May 19, 2017 •

edited

Loading

mkonicek May 22, 2017 •

edited

Loading

mkonicek May 22, 2017 •

edited

Loading

mkonicek Jun 1, 2017 •

edited

Loading

mkonicek May 22, 2017 •

edited

Loading

mkonicek commented May 22, 2017 •

edited

Loading

mkonicek commented May 30, 2017 •

edited

Loading

hey99xx commented Jan 9, 2018 •

edited

Loading

hey99xx commented Feb 1, 2018 •

edited

Loading