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
base: main
Are you sure you want to change the base?
Conversation
Base commit: 47848ad |
@rshest has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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
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
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
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
/** 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 |
There was a problem hiding this comment.
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
?
@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 @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:
Go depth into the implementation. In general, we want to reuse the 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. |
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. |
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 replaceDevInternalSettings
inside expo-dev-client because we cannot access to theDevInternalSettings
anymore because Kotlin "internal" visibility.This PR tries to decouple
DevInternalSettings
fromDevSupportManagerBase
then we could still use reflection to change the mDevSettings.Changelog:
[ANDROID] [CHANGED] - Decouple
DevInternalSettings
fromDevSupportManagerBase
Test Plan:
CI passed