[reupload] - added: Websocket Module setCustomClientBuilder #37798

MO-Lewis · 2023-06-09T12:37:58Z

Summary:

Added code previously added in the following PR: #28659.

The above PR was accidentally scrapped due to the renaming of the master branch on the React Native repo. As advised in this comment: #37770 (comment), I'm opening a new PR with the same code to get this merged into master / main.

Currently, we need to run a local fork of React Native and manually apply these changes ourselves. This then causes additional issues, as it's currently impossible to build React Native from source when running on a Windows machine, as evidenced in #37770 and the other linked issues nested inside of this issue.

Original summary is as follows:
With NetworkModule.setCustomClientBuilder we can customize our OkHttpClient to all requests made by react-native, it's very useful when you do SSL Pinning or change some OkHttpClient configuration at all. I've added a similar function to websocket, it allow us do some configurations on Websocket OkHttpClient.

Changelog:

[Android] [Added] - Websocket Module setCustomClientBuilder

Test Plan:

From the original PR:

You can just set a custom CustomClientBuilder on MainActivity onCreate:

import okhttp3.OkHttpClient;
import java.util.concurrent.TimeUnit;
import com.facebook.react.modules.network.CustomClientBuilder;
import com.facebook.react.modules.websocket.WebsocketModule;

public class MainActivity extends ReactFragmentActivity {

    @Override
    protected void onCreate(Bundle savedInstanceState) {
        super.onCreate(savedInstanceState);
        RNBootSplash.init(R.drawable.launch_screen, MainActivity.this);

        WebsocketModule.setCustomClientBuilder(new CustomClientBuilder() {
            @Override
            public void apply(OkHttpClient.Builder builder) {
              builder.connectTimeout(0, TimeUnit.MILLISECONDS);
            }
          });
    }

   ...

facebook-github-bot · 2023-06-09T12:38:03Z

Hi @MO-Lewis!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

analysis-bot · 2023-06-09T13:04:23Z

Platform	Engine	Arch	Size (bytes)	Diff
android	hermes	arm64-v8a	8,836,393	+144
android	hermes	armeabi-v7a	8,146,280	+140
android	hermes	x86	9,341,760	+135
android	hermes	x86_64	9,184,507	+144
android	jsc	arm64-v8a	9,448,196	+89
android	jsc	armeabi-v7a	8,629,957	+93
android	jsc	x86	9,530,877	+95
android	jsc	x86_64	9,774,080	+92

Base commit: 04ad34d
Branch: main

MO-Lewis · 2023-06-20T14:01:10Z

FYI I've just signed the CLA, I'm guessing it'll update the PR shortly 😅

facebook-github-bot · 2023-06-20T15:08:42Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

javache · 2023-07-07T08:34:35Z

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

-  public static interface CustomClientBuilder {
-    public void apply(OkHttpClient.Builder builder);
-  }


For backwards compatibility, can you keep the old interface name here, but mark it as deprecated. We can then remove it in a future release

public static interface CustomClientBuilder extends com.facebook.react.modules.network.CustomClientBuilder {}

javache · 2023-07-07T08:35:30Z

Please note the deprecation of the old interface in the changelog entry, other than that, this seems good to me.

…ecation, as suggested here: facebook#37798 (review)

MO-Lewis · 2023-07-11T10:16:40Z

Hi @javache! I've just made that change for you, I believe this is what you wanted. If there's something not right about the change I've made, or have any additional feedback, please let me know. 😄

Thanks!

…up to commit `52698e758d1454afc74541e5507ac16dd6154386` Merge remote-tracking branch 'upstream/main'

MO-Lewis · 2023-07-11T10:31:29Z

Just did another commit to merge everything and pull recent changes from the main React Native repo up to commit 52698e758d1454afc74541e5507ac16dd6154386

javache · 2023-07-11T13:13:24Z

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

+  /**
+   * @deprecated To be removed in a future release. See https://github.com/facebook/react-native/pull/37798#pullrequestreview-1518338914
+   */
+  public static interface CustomClientBuilder extends com.facebook.react.modules.network.CustomClientBuilder {
    public void apply(OkHttpClient.Builder builder);


This can just be an empty interface.

Implemented in latest commit; resolve if sufficient.

javache · 2023-07-11T13:13:33Z

...ative/ReactAndroid/src/main/java/com/facebook/react/modules/network/CustomClientBuilder.java

+
+public interface CustomClientBuilder {
+  public void apply(OkHttpClient.Builder builder);
+}


nit: empty new line at end of file

Amended in latest commit; resolve if sufficient.

javache

Looking good, few more changes.

javache · 2023-07-11T13:13:56Z

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

+  /**
+   * @deprecated To be removed in a future release. See https://github.com/facebook/react-native/pull/37798#pullrequestreview-1518338914
+   */
+  public static interface CustomClientBuilder extends com.facebook.react.modules.network.CustomClientBuilder {


Make sure the API's in this class reference com.facebook.react.modules.network.CustomClientBuilder, not this CustomClientBuilder.

Apologies, I'm not too sure what you mean by this. Does this mean you'd like com.facebook.react.modules.network.CustomClientBuilder explicitly imported at the top of this file?

Anywhere in this Java file where it references CustomClientBuilder it will use the old deprecated interface, you could point at the shared interface instead, and will prevent people from upgrading their code.

Hi @javache, I believe I've implemented what you'd like. I've changed:

public static void setCustomClientBuilder(CustomClientBuilder ccb) {
to
public static void setCustomClientBuilder(com.facebook.react.modules.network.CustomClientBuilder ccb) {

Does this cover what you've requested? Anything else, just let me know 🙂

javache · 2023-07-11T13:14:22Z

packages/rn-tester/android/app/src/debug/java/com/facebook/react/uiapp/ReactNativeFlipper.java

@@ -0,0 +1,75 @@
+/*


This file has been incorrectly merged back in.

Apologies! Removed in latest commit; resolve if sufficient.

Changelog: - Added empty new line at end of file on `CustomClientBuilder.java` - Removed accidentally-added `ReactNativeFlipper.java` file. - Made deprecated `CustomClientBuilder` interface empty.

javache · 2023-07-12T16:38:48Z

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

@@ -167,9 +167,10 @@ public static void setCustomClientBuilder(CustomClientBuilder ccb) {
    customClientBuilder = ccb;


Silly github doesn't allow you to comment on unchanged lines, but line 168 here needs to be com.facebook.react.modules.network.CustomClientBuilder

Hi Javache, I've implemented and replied to this here

Changelog: - Implemented feedback to point all `CustomClientBuilder` references in `NetworkingModule,java` to `com.facebook.react.modules.network.CustomClientBuilder`

javache

The type of customClientBuilder in NetworkingModule also needs to become com.facebook.react.modules.network.CustomClientBuilder

Changelog: - Changed `private static @nullable CustomClientBuilder` to `private static @nullable com.facebook.react.modules.network.CustomClientBuilder`

MO-Lewis · 2023-07-14T09:46:18Z

The type of customClientBuilder in NetworkingModule also needs to become com.facebook.react.modules.network.CustomClientBuilder

Thanks for the heads up! I've just made that change now.

facebook-github-bot · 2023-07-14T11:33:02Z

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

cortinico

@MO-Lewis could you rebase on top of the latest main as the CI should be green now

MO-Lewis · 2023-07-18T09:25:19Z

@MO-Lewis could you rebase on top of the latest main as the CI should be green now

All done! CI is rerunning for you now.

cortinico

One nit @MO-Lewis and we should be able to merge it

cortinico · 2023-07-18T10:11:19Z

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

+  /**
+   * @deprecated To be removed in a future release. See https://github.com/facebook/react-native/pull/37798#pullrequestreview-1518338914
+   */
+  public static interface CustomClientBuilder extends com.facebook.react.modules.network.CustomClientBuilder {}


Can we also @Deprecate this method?

Sure! Just added that, CICD re-running now.

Changelog: - Added `@Deprecated` to `CustomClientBuilder` in `NetworkingModule.java

Changelog: - Added `@Deprecated` to `CustomClientBuilder` in `NetworkingModule.java`

…m-ws-factory

facebook-github-bot · 2023-07-18T13:18:51Z

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

facebook-github-bot · 2023-07-18T21:01:05Z

@javache merged this pull request in 0cdb9e6.

Summary: Added code previously added in the following PR: facebook#28659. The above PR was accidentally scrapped due to the renaming of the master branch on the React Native repo. As advised in this comment: facebook#37770 (comment), I'm opening a new PR with the same code to get this merged into master / main. Currently, we need to run a local fork of React Native and manually apply these changes ourselves. This then causes additional issues, as it's currently _**impossible**_ to build React Native from source when running on a Windows machine, as evidenced in facebook#37770 and the other linked issues nested inside of this issue. **Original summary is as follows:** With `NetworkModule.setCustomClientBuilder` we can customize our OkHttpClient to all requests made by react-native, it's very useful when you do `SSL Pinning` or change some OkHttpClient configuration at all. I've added a similar function to websocket, it allow us do some configurations on Websocket OkHttpClient. ## Changelog: [Android] [Added] - Websocket Module setCustomClientBuilder <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#37798 Test Plan: **From the original PR:** You can just set a custom `CustomClientBuilder` on `MainActivity` `onCreate`: ``` import okhttp3.OkHttpClient; import java.util.concurrent.TimeUnit; import com.facebook.react.modules.network.CustomClientBuilder; import com.facebook.react.modules.websocket.WebsocketModule; public class MainActivity extends ReactFragmentActivity { Override protected void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); RNBootSplash.init(R.drawable.launch_screen, MainActivity.this); WebsocketModule.setCustomClientBuilder(new CustomClientBuilder() { Override public void apply(OkHttpClient.Builder builder) { builder.connectTimeout(0, TimeUnit.MILLISECONDS); } }); } ... ``` Reviewed By: cortinico Differential Revision: D47468613 Pulled By: javache fbshipit-source-id: ad97fb18ba5784d8abe157f5ccd29201b8b0fe84

Summary: Added code previously added in the following PR: facebook#28659. The above PR was accidentally scrapped due to the renaming of the master branch on the React Native repo. As advised in this comment: facebook#37770 (comment), I'm opening a new PR with the same code to get this merged into master / main. Currently, we need to run a local fork of React Native and manually apply these changes ourselves. This then causes additional issues, as it's currently _**impossible**_ to build React Native from source when running on a Windows machine, as evidenced in facebook#37770 and the other linked issues nested inside of this issue. **Original summary is as follows:** With `NetworkModule.setCustomClientBuilder` we can customize our OkHttpClient to all requests made by react-native, it's very useful when you do `SSL Pinning` or change some OkHttpClient configuration at all. I've added a similar function to websocket, it allow us do some configurations on Websocket OkHttpClient. [Android] [Added] - Websocket Module setCustomClientBuilder <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: [ANDROID|GENERAL|IOS|INTERNAL] [BREAKING|ADDED|CHANGED|DEPRECATED|REMOVED|FIXED|SECURITY] - Message For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests Pull Request resolved: facebook#37798 Test Plan: **From the original PR:** You can just set a custom `CustomClientBuilder` on `MainActivity` `onCreate`: ``` import okhttp3.OkHttpClient; import java.util.concurrent.TimeUnit; import com.facebook.react.modules.network.CustomClientBuilder; import com.facebook.react.modules.websocket.WebsocketModule; public class MainActivity extends ReactFragmentActivity { Override protected void onCreate(Bundle savedInstanceState) { super.onCreate(savedInstanceState); RNBootSplash.init(R.drawable.launch_screen, MainActivity.this); WebsocketModule.setCustomClientBuilder(new CustomClientBuilder() { Override public void apply(OkHttpClient.Builder builder) { builder.connectTimeout(0, TimeUnit.MILLISECONDS); } }); } ... ``` Reviewed By: cortinico Differential Revision: D47468613 Pulled By: javache fbshipit-source-id: ad97fb18ba5784d8abe157f5ccd29201b8b0fe84

Added code previously added in the following PR: facebook#28659

7232fbb

MO-Lewis mentioned this pull request Jun 9, 2023

Execution failed for task ':react-native:ReactAndroid:hermes-engine:configureBuildForHermes'. #37780

Closed

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 Jun 20, 2023

javache reviewed Jul 7, 2023

View reviewed changes

Readded old interface name for CustomClientBuilder, marked for depr…

b741ef3

…ecation, as suggested here: facebook#37798 (review)

Note: Merged latest commits from main React Native repo. Merging all …

83d06fb

…up to commit `52698e758d1454afc74541e5507ac16dd6154386` Merge remote-tracking branch 'upstream/main'

javache reviewed Jul 11, 2023

View reviewed changes

javache requested changes Jul 11, 2023

View reviewed changes

Implemented requested feedback from javache.

bd85807

Changelog: - Added empty new line at end of file on `CustomClientBuilder.java` - Removed accidentally-added `ReactNativeFlipper.java` file. - Made deprecated `CustomClientBuilder` interface empty.

MO-Lewis requested a review from javache July 12, 2023 16:06

javache reviewed Jul 12, 2023

View reviewed changes

Implemented requested feedback from javache.

eeb5479

Changelog: - Implemented feedback to point all `CustomClientBuilder` references in `NetworkingModule,java` to `com.facebook.react.modules.network.CustomClientBuilder`

MO-Lewis requested a review from javache July 13, 2023 16:09

javache approved these changes Jul 14, 2023

View reviewed changes

javache requested changes Jul 14, 2023

View reviewed changes

Implemented requested feedback from javache.

0098fc9

Changelog: - Changed `private static @nullable CustomClientBuilder` to `private static @nullable com.facebook.react.modules.network.CustomClientBuilder`

MO-Lewis requested a review from javache July 14, 2023 09:46

javache approved these changes Jul 14, 2023

View reviewed changes

cortinico reviewed Jul 17, 2023

View reviewed changes

Merge branch 'main' of https://github.com/facebook/react-native

7129a85

cortinico approved these changes Jul 18, 2023

View reviewed changes

MO-Lewis added 3 commits July 18, 2023 12:12

Implemented requested feedback from cortinico.

067554b

Changelog: - Added `@Deprecated` to `CustomClientBuilder` in `NetworkingModule.java

Implemented requested feedback from cortinico.

535613c

Changelog: - Added `@Deprecated` to `CustomClientBuilder` in `NetworkingModule.java`

Merge branch 'main' of https://github.com/MO-Lewis/react-native-custo…

37c1fa1

…m-ws-factory

facebook-github-bot closed this in 0cdb9e6 Jul 18, 2023

facebook-github-bot added the Merged This PR has been merged. label Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[reupload] - added: Websocket Module setCustomClientBuilder #37798

[reupload] - added: Websocket Module setCustomClientBuilder #37798

MO-Lewis commented Jun 9, 2023

facebook-github-bot commented Jun 9, 2023

analysis-bot commented Jun 9, 2023 •

edited

Loading

MO-Lewis commented Jun 20, 2023

facebook-github-bot commented Jun 20, 2023

javache Jul 7, 2023

javache commented Jul 7, 2023

MO-Lewis commented Jul 11, 2023

MO-Lewis commented Jul 11, 2023

javache Jul 11, 2023

MO-Lewis Jul 11, 2023

javache Jul 11, 2023

MO-Lewis Jul 11, 2023

javache left a comment

javache Jul 11, 2023

MO-Lewis Jul 11, 2023

javache Jul 12, 2023

MO-Lewis Jul 13, 2023

javache Jul 11, 2023

MO-Lewis Jul 11, 2023

javache Jul 12, 2023

MO-Lewis Jul 13, 2023

javache left a comment

MO-Lewis commented Jul 14, 2023

facebook-github-bot commented Jul 14, 2023

cortinico left a comment

MO-Lewis commented Jul 18, 2023

cortinico left a comment

cortinico Jul 18, 2023

MO-Lewis Jul 18, 2023

facebook-github-bot commented Jul 18, 2023

facebook-github-bot commented Jul 18, 2023

		@@ -167,9 +167,10 @@ public static void setCustomClientBuilder(CustomClientBuilder ccb) {
		customClientBuilder = ccb;

[reupload] - added: Websocket Module setCustomClientBuilder #37798

[reupload] - added: Websocket Module setCustomClientBuilder #37798

Conversation

MO-Lewis commented Jun 9, 2023

Summary:

Changelog:

Test Plan:

facebook-github-bot commented Jun 9, 2023

Action Required

Process

analysis-bot commented Jun 9, 2023 • edited Loading

MO-Lewis commented Jun 20, 2023

facebook-github-bot commented Jun 20, 2023

Choose a reason for hiding this comment

javache commented Jul 7, 2023

MO-Lewis commented Jul 11, 2023

MO-Lewis commented Jul 11, 2023

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

javache left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javache left a comment

Choose a reason for hiding this comment

MO-Lewis commented Jul 14, 2023

facebook-github-bot commented Jul 14, 2023

cortinico left a comment

Choose a reason for hiding this comment

MO-Lewis commented Jul 18, 2023

cortinico left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

facebook-github-bot commented Jul 18, 2023

facebook-github-bot commented Jul 18, 2023

analysis-bot commented Jun 9, 2023 •

edited

Loading