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

Decouple DevInternalSettings from DevSupportManagerBase #44441

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented May 7, 2024

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

@facebook-github-bot 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. Contributor A React Native contributor. p: Expo Partner: Expo Partner labels May 7, 2024
@analysis-bot
Copy link

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

@Kudo Kudo marked this pull request as ready for review May 7, 2024 13:41
@facebook-github-bot facebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label May 7, 2024
@facebook-github-bot
Copy link
Contributor

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

rshest pushed a commit to rshest/react-native that referenced this pull request May 8, 2024
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 facebook@a977b2e69, we cannot reuse the `DevSupportManagerBase` and replace `DevInternalSettings` inside [expo-dev-client](https://github.com/expo/expo/blob/26c9f49042f53db7d37f832c133d4da0f6d64f02/packages/expo-dev-launcher/android/src/debug/java/expo/modules/devlauncher/helpers/DevLauncherReactUtils.kt#L117-L126) 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`

Pull Request resolved: facebook#44441

Test Plan: CI passed

Differential Revision: https://internalfb.com/D57054234
rshest pushed a commit to rshest/react-native that referenced this pull request May 8, 2024
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 facebook@a977b2e69, we cannot reuse the `DevSupportManagerBase` and replace `DevInternalSettings` inside [expo-dev-client](https://github.com/expo/expo/blob/26c9f49042f53db7d37f832c133d4da0f6d64f02/packages/expo-dev-launcher/android/src/debug/java/expo/modules/devlauncher/helpers/DevLauncherReactUtils.kt#L117-L126) 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`

Pull Request resolved: facebook#44441

Test Plan: CI passed

Differential Revision: https://internalfb.com/D57054234
rshest pushed a commit to rshest/react-native that referenced this pull request May 8, 2024
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 facebook@a977b2e69, we cannot reuse the `DevSupportManagerBase` and replace `DevInternalSettings` inside [expo-dev-client](https://github.com/expo/expo/blob/26c9f49042f53db7d37f832c133d4da0f6d64f02/packages/expo-dev-launcher/android/src/debug/java/expo/modules/devlauncher/helpers/DevLauncherReactUtils.kt#L117-L126) 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`

Pull Request resolved: facebook#44441

Test Plan: CI passed

Differential Revision: https://internalfb.com/D57054234
rshest pushed a commit to rshest/react-native that referenced this pull request May 8, 2024
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 facebook@a977b2e69, we cannot reuse the `DevSupportManagerBase` and replace `DevInternalSettings` inside [expo-dev-client](https://github.com/expo/expo/blob/26c9f49042f53db7d37f832c133d4da0f6d64f02/packages/expo-dev-launcher/android/src/debug/java/expo/modules/devlauncher/helpers/DevLauncherReactUtils.kt#L117-L126) 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`

Pull Request resolved: facebook#44441

Test Plan: CI passed

Differential Revision: https://internalfb.com/D57054234
Comment on lines -13 to +18
/** whether an overlay showing current FPS should be shown. */
public fun isFpsDebugEnabled(): Boolean
/** Whether an overlay showing current FPS should be shown. */
public var isFpsDebugEnabled: Boolean
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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
Copy link
Contributor

rshest commented May 8, 2024

@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
Copy link
Contributor Author

Kudo commented May 9, 2024

@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
Copy link
Contributor

@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
Copy link
Contributor Author

Kudo commented May 14, 2024

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.

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. Contributor A React Native contributor. p: Expo Partner: Expo Partner Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants