[expo-dev-client][expo-modules-core] Android automatic setup of dev client #16441
Conversation
… listeners to debug
| * @return a ReactRootView instance to be used as a container, or null if no container is needed | ||
| */ | ||
| @Nullable | ||
| default ReactRootView createReactRootViewContainer(Activity activity) { |
There was a problem hiding this comment.
is the container necessary to be a ReactRootView? it would be better if ViewGroup is enough. i am afraid if there are any problems for nested ReactRootView.
There was a problem hiding this comment.
oh i got the reason, because createReactRootView requires a ReactRootView but not a ViewGroup:
how about we insert the ReactRootView container in loadApp?
https://github.com/facebook/react-native/blob/189c2c8958442541c6b4f42860b2943ece612da2/ReactAndroid/src/main/java/com/facebook/react/ReactActivityDelegate.java#L92
There was a problem hiding this comment.
cool idea. this works 🎉
| .mapNotNull { it.onDidCreateReactActivityDelegate(activity, this) } | ||
| .firstOrNull() | ||
| if (newDelegate != null && newDelegate != this) { | ||
| val mDelegateField = ReactActivity::class.java.getDeclaredField("mDelegate") |
There was a problem hiding this comment.
proguard rules should be updated for accessing ReactActivity.mDelegate
| return delegate.onKeyUp(keyCode, event) | ||
| return reactActivityHandlers | ||
| .map { it.onKeyUp(keyCode, event) } | ||
| .fold(false) { accu, current -> accu || current } || delegate.onKeyUp(keyCode, event) |
There was a problem hiding this comment.
it's tricky here and i always being confused if i did right. taking onNewIntent as an example:
override fun onNewIntent(intent: Intent?): Boolean {
val listenerResult = reactActivityLifecycleListeners
.map { it.onNewIntent(intent) }
.fold(false) { accu, current -> accu || current }
val delegateResult = delegate.onNewIntent(intent)
return listenerResult || delegateResult
}where i intentionally separate as two difference variables. even if a listener returns true, i think we should still call delegate.onNewIntent.
in your case, if a listener returns true from onKeyUp, ReactDelegate.onKeyUp will not be called. i'm afraid there will be something broken from react-native's key handler.
There was a problem hiding this comment.
(ignore my previous comment here, I messed up when I was testing 😅 )
Consuming the key event and not letting it through to ReactDelegate.onKeyUp is actually purposeful here, as this is the way that we currently override RN's built in dev menu. Otherwise the shake gesture brings up both RN's and our dev menu for a split second. DevMenuAwareReactActivity is already written like this so I don't think this should cause issues from the dev menu specifically. But I understand your concern for the more general case 😕 I will at least document this in a comment, do you think that's sufficient?
esamelson
force-pushed
the
@eric/eng-2611-android-0
branch
from
f173e3e
to
4be5c29
Compare
expo-bot
added
bot: passed checks
Why
ENG-2611 and ENG-4209 for Android / Android version of #16190
Integrating expo-dev-launcher with the expo-modules automatic setup system is long overdue and will fix a whole host of issues with the current system, which relies heavily on unsafe regexes in the config plugin.
How
Use DevLauncherPackage and DevMenuPackage to hook into the expo modules wrapper system and replace all of the existing integrations with MainApplication/MainActivity.
In order to do so, added a few things to the ReactActivityDelegateWrapper and ReactActivityHandler (discussed via Slack and in #16304):
Everything else was achievable with methods already in place in the expo-modules system 🎉
This new integration is only additive and is fully backwards-compatible. All of the integrations will no-op if the project is already using the old config-plugin-based system. Also, if the project is using SDK 44 or older, the dev client will still compile, as the new handler code is siloed based on the
expopackage version.Test Plan
Tested the following scenarios manually:
In a project without the current manual integration (expo init bare, install local dev-client, resolve local expo/modules-core, add
expoPackageVersion = "45.0.0"to android/build.gradle):✅ dev launcher runs in a debug build
✅ can open locally served project from expo-cli
✅ shake and three finger touch both open and close the dev menu
✅ can go back to launcher, then open project again
✅ can use a deep link (via QR code) to open project while app is running
✅ can use a deep link (via QR code) to open app, it loads the project correctly
✅ release build does not have the dev client
In a project with the current manual integration (expo init blank, install local dev-client, resolve local expo/modules-core, prebuild), this new one shouldn't interfere:
✅ shouldEnableAutoSetup evaluates to false in both dev-launcher and dev-menu
✅ all of the above test cases still pass
Checklist
expo build(eg: updated@expo/xdl).expo prebuild& EAS Build (eg: updated a module plugin).