react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Decouple DevInternalSettings from DevSupportManagerBase

Open Kudo opened this issue 1 month ago • 6 comments

Summary:

I was tried to fix breaking changes for Expo's React Native nightlies CI testing. Recently React Native core has some effort to migrate Java code to Kotlin. Since https://github.com/facebook/react-native/commit/a977b2e69, we cannot reuse the DevSupportManagerBase and replace DevInternalSettings inside expo-dev-client because we cannot access to the DevInternalSettings anymore because Kotlin "internal" visibility. This PR tries to decouple DevInternalSettings from DevSupportManagerBase then we could still use reflection to change the mDevSettings.

Changelog:

[ANDROID] [CHANGED] - Decouple DevInternalSettings from DevSupportManagerBase

Test Plan:

CI passed

Kudo avatar May 07 '24 12:05 Kudo

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 19,495,298 +11
android hermes armeabi-v7a n/a --
android hermes x86 n/a --
android hermes x86_64 n/a --
android jsc arm64-v8a 22,867,622 +10
android jsc armeabi-v7a n/a --
android jsc x86 n/a --
android jsc x86_64 n/a --

Base commit: 47848ad15f243b277460be00ff407c12b176557f Branch: main

analysis-bot avatar May 07 '24 12:05 analysis-bot

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

facebook-github-bot avatar May 07 '24 15:05 facebook-github-bot

@Kudo, this (understandably) got a bit of pushback internally, as with this change we effectively make many more things public (and, as @alanleedev noted, not read-only anymore).

Can we work together on what is that Expo trying to achieve via using reflection to get to the internals, exactly?

The purpose of internals are to be... well, internal.

What are the exact things that are missing from the public interface right now?

I am curious whether we could reduce the blast radius here and only expose things that are absolutely needed.

rshest avatar May 08 '24 18:05 rshest

@rshest @alanleedev It's 100% understandable and I was expecting that you should have concern for this PR. I'm happy to discuss in depth.

Let's walk through our need in high level. The expo-dev-client has two underlying modules:

  • expo-dev-launcher - a module that could launch different apps, that's the example.
    • the launcher could restart apps and load bundles from different servers, e.g. metro servers running on colleagues' hosts or even EAS Update servers.
    • we need a programatic way to specify PackagerConnection/InspectorPackagerConnection address.
    • we need to control when to start/stop PackagerConnection/InspectorPackagerConnection.
    • we also have customized UI than the default RedBox.
  • expo-dev-menu - a customized DevMenu, this is the example.
    • since we would load different apps from different places, we have a customized DeveloperSetting than store every settings on Android SharedPreferences.

Go depth into the implementation. In general, we want to reuse the DevSupportManagerBase code since it's frequently changed. With DevSupportManagerBase, we need to customize its DevInternalSettings to control PackagerConnection.

We look forward to seeing React Native could split out these devsupport features from core and frameworks could have a way to customize these devsupport features. But that's not an easy task in the moment.

Hopes that's clear to you and let's see if we together could brainstorm a way to access to these internal and also balanced for maintenance.

Kudo avatar May 09 '24 13:05 Kudo

@Kudo Thanks for the detailed explanation. @rshest is on vacation and also didn't get much internal input on this issue. As a quick workaround, could we undo the internal visibility for DevInternalSettings and making it public?

alanleedev avatar May 14 '24 18:05 alanleedev

that's just for nightlies testing so no hurry. we could wait longer and makes sure we could find a solution that we are happy with.

Kudo avatar May 14 '24 19:05 Kudo