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

[dev-client][splash-screen] Fix error when running with new arch mode on android #19931

Merged
merged 3 commits into from
Nov 8, 2022

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Nov 8, 2022

Why

when turning on new architecture mode with dev-client, there is a runtime crash:

java.lang.AssertionError: TurboModules are enabled, but mTurboModuleRegistry hasn't been set.

fixes #19866

How

  • react-native has a singleton flag ReactFeatureFlags.useTurboModules. even though we have the ReactNativeHost with old architecture mode in dev-menu and dev-launcher, the underlying CatalystInstanceImpl will still fail at the check. the ReactPackageTurboModuleManagerDelegate.Builder requires some c++ code, considering we don't support new architecture mode in dev-menu and dev-launcher, this pr uses a workaround to pass the ReactPackageTurboModuleManagerDelegate.Builder from MainApplication.

  • when running on new architecture mode, there is a NPE in FrameLayout.onMeasure. since splash screen view remove itself in ViewGroup.OnHierarchyChangeListener. the child iteration will get null from getChildAt(). the pr tries to remove the splash screen view from the next run loop.

Test Plan

Checklist

Sorry, something went wrong.

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Nov 8, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Nov 8, 2022
Copy link
Contributor

@lukmccall lukmccall left a comment

Choose a reason for hiding this comment

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

Nice 🔥
I'll release a dev-client after you merge it ;)

@Kudo Kudo marked this pull request as ready for review November 8, 2022 14:59
@Kudo Kudo merged commit b0ad091 into main Nov 8, 2022
@Kudo Kudo deleted the @kudo/sdk47/fix-dev-client-new-arch branch November 8, 2022 16:10
tsapeta pushed a commit that referenced this pull request Nov 8, 2022
gabrieldonadel added a commit that referenced this pull request May 23, 2023
# Why

Closes ENG-7955

# How

Implement the missing `getJSIModulePackage` methods to both
`ReactNativeHost`s in order to support the new architecture.
`ReactPackageTurboModuleManagerDelegate` was already implemented on
#19931

# Test Plan

Run `fabric-tester` and `bare-expo` apps on Android

# Checklist

<!--
Please check the appropriate items below if they apply to your diff.
This is required for changes to Expo modules.
-->

- [ ] Documentation is up to date to reflect these changes (eg:
https://docs.expo.dev and README.md).
- [ ] Conforms with the [Documentation Writing Style
Guide](https://github.com/expo/expo/blob/main/guides/Expo%20Documentation%20Writing%20Style%20Guide.md)
- [ ] This diff will work correctly for `npx expo prebuild` & EAS Build
(eg: updated a module plugin).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: passed checks ExpoBot has nothing to complain about
Projects
None yet
3 participants