Decouple DevInternalSettings from DevSupportManagerBase

[Kudo]

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 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

[analysis-bot]

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: 47848ad
Branch: main

[facebook-github-bot]

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

[alanleedev]

@Kudo This question applies to all fun -> var changes, but it appears as you are taking a read-only interface and making it read-write for everyone. Is this a safe change to make?
Also if the internal visibility DevInternalSettings is of an issue, would it be just better to make it public?

[rshest]

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

[Kudo]

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

[alanleedev]

@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?

[Kudo]

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]

three weeks past and would be good to hear this again from you.

[facebook-github-bot]

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

[cortinico]

As a quick workaround, could we undo the internal visibility for DevInternalSettings and making it public?

Jumping here as I originally made the class internal:

I think it's safe to land this change that makes DeveloperSettings a read/write interface as that's the purpose of that class (allowing developers to toggle property from dev menus and so on).

Developers till 0.74 could achieve the same results by just accessing DevInternalSettings or even by updating the SharedPreferences directly.

I'd rather have a cleaner interface with read/write var rather than allowing developers to keep accessing *Internal* classes which leaks implementation details

[Kudo]

I'd rather have a cleaner interface with read/write var rather than allowing developers to keep accessing Internal classes which leaks implementation details

exactly, the main effort was not about read/write var but to clean the Internal class from DevSupportManagerBase. if we could have other idea than read/write var to decouple DevInternalSettings from DevSupportManagerBase. i'd happy to try the change.

[facebook-github-bot]

@cortinico merged this pull request in 52cec1e.

[github-actions]

This pull request was successfully merged by @Kudo in 52cec1e.

^{When will my fix make it into a release? | How to file a pick request?}