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

[core] add getJSBundleFile support for bridgeless mode #27804

Merged
merged 2 commits into from
Mar 26, 2024

Conversation

Kudo
Copy link
Contributor

@Kudo Kudo commented Mar 22, 2024

Why

expo-updates uses the ReactNativeHost.getJSBundleFile() to define the launch bundle path. the defaultReactHost doesn't support it, so expo-updates always uses the embedded bundle.

How

reference the logic from bridged ReactInstanceManager to support the getJSBundleFile(): https://github.com/facebook/react-native/blob/15a5638c621cbec4a9fcb5ae94938120cdc32fae/packages/react-native/ReactAndroid/src/main/java/com/facebook/react/ReactInstanceManagerBuilder.java#L107-L114

Test Plan

test canary + expo-updates + eas updates that should launch the remote bundle

Checklist

@expo-bot expo-bot added the bot: suggestions ExpoBot has some suggestions label Mar 22, 2024
@expo-bot
Copy link
Collaborator

expo-bot commented Mar 22, 2024

The Pull Request introduced fingerprint changes against the base commit: a745c5e

Fingerprint diff
[
  {
    "type": "dir",
    "filePath": "../../packages/expo/android",
    "reasons": [
      "expoAutolinkingAndroid"
    ],
    "hash": "15a1b88268d183e2033498e5180e5c4a4eac6802"
  }
]

Generated by PR labeler 🤖

@expo-bot expo-bot added bot: passed checks ExpoBot has nothing to complain about and removed bot: suggestions ExpoBot has some suggestions labels Mar 22, 2024
@Kudo Kudo marked this pull request as ready for review March 22, 2024 08:57
@Kudo Kudo requested review from lukmccall and removed request for brentvatne and ide March 22, 2024 10:58
Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

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

@wschurman adding you as a post-commit reviewer so you're aware of this change. It should be relatively quick to look at.

@@ -41,6 +41,12 @@ object ExpoReactHostFactory {
override val jsBundleLoader: JSBundleLoader
get() {
val context = weakContext.get() ?: throw IllegalStateException("Unable to get concrete Context")
reactNativeHostWrapper.jsBundleFile?.let { jsBundleFile ->
Copy link
Member

Choose a reason for hiding this comment

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

Would it be helpful to add comments saying which parts of the code run in bridgeless mode vs. with the bridge enabled? Later, when there is only bridgeless mode, we can remove the comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good question! basically this whole ExpoReactHostFactory file is for bridgeless. react-native has a convention

  • bridgeless: ReactHost, ReactInstance
  • bridge: ReactNativeHost, ReactInstanceMananger, Catalyst*

there's some effort to adapt from ReactNativeHost to ReactHost without much change. in this example, it reads some necessary properties from ReactNativeHost.

basically i would try to follow the same effort to migrate our ReactNativeHostHandler to be bridgeless/bridge independent, so that expo modules don't have to handle these different cases. though expo-dev-client and expo-updates are two special cases that they are some tight coupled code inside. we are aware and working on the migration.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay. Thanks for the explanation. If I understand correctly, does that mean that when bridged mode is removed (one day, in the future), we'll move the code from ReactNativeHostWrapper into ReactHostFactory, since ReactNativeHost will no longer exist?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I understand correctly, does that mean that when bridged mode is removed (one day, in the future), we'll move the code from ReactNativeHostWrapper into ReactHostFactory, since ReactNativeHost will no longer exist?

right! we will merge into one class. the class name might be ReactHostWrapper, depending on how MainApplication from the template changes over time. if the logic for ReactNativeHost moved to ReactHost, then we may have a ReactHostWrapper after all.

@Kudo Kudo requested a review from wschurman March 22, 2024 15:33
@Kudo Kudo merged commit 77d8a7c into main Mar 26, 2024
20 checks passed
@Kudo Kudo deleted the @kudo/bridgeless/fix-updates-android branch March 26, 2024 17:12
@brentvatne brentvatne added the published Changes from the PR have been published to npm label Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: fingerprint changed bot: passed checks ExpoBot has nothing to complain about published Changes from the PR have been published to npm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants