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

Apennine Architecture - add support for multiple bundle on a single JSContext #25518

Closed
wants to merge 26 commits into from

Conversation

zamotany
Copy link

@zamotany zamotany commented Jul 5, 2019

Summary

This PR is a implementation of multi-bundle support in a single JSContext, which is described here - react-native-community/discussions-and-proposals#127

It's a joint effort of me and @dratwas.

Target audience

The multi-bundle mode is targeted mainly at corporate/large companies, where smaller teams are responsible for each piece of functionality - a mini app. More info about motivation can be found in the RFC.

Smaller teams or developers creating small/medium sized apps, should stick to the single-bundle mode, which is what React Native and Metro support currently. Because of that, multi-bundle is a opt-in feature.

Backwards compatibility

The new architecture is fully compatible with Metro. The plan JS bundles as well as Indexed RAM bundle, File RAM Bundle and Delta Bundle are working as expected. Remote debugging in Chrome is working properly.

It's more difficult to account for 3rd party libraries, which are going to break, but it's likely that react-native-code-push will stop working and will require some work.

JavaScript API

To allow to request new bundle from JS, BundleRegistry JS module is now available publicly. The additional bundles can request to be loaded either synchronously or asynchronously. Only the initial bundle is automatically loaded from the native side.

New architecture

In order to add maintainable and scalable support for multi-bundle we had to modify the core of React Native's logic for loading and evaluating the JS bundles. We've laid a ground work for adding support to have other bundles running on different JSContextes and threads.

Each bundle type has it's own C++ class:

  • BasicBundle - plain JS bundle, where JS code is stored as JSBigString
  • IndexedRAMBundle - single-file RAM bundle (CxxReact)
  • FileRAMBundle - multi-file RAM bundle (ReactAndroid)
  • DeltaBundle - delta bundle for development (CxxReact)

All of those bundles derive from abstract Bundle class in CxxReact.

Since the new bundles can requested on-demand, instead of taking a single bundle in RCTCxxBridge or CatalystInstance it takes a loader, which has enough information to load initial bundle and additional bundles:

  • RCTDevBundleLoader - loader for bundles served from packager server (React)
  • RCTFileBundleLoader - loader for bundles stored in filesystem (React)
  • AssetBundleLoader - loader for bundles stored as assets (ReactAndroid)
  • FileBundleLoader - loader for bundles stored in filesystem (ReactAndroid)
  • DeltaBundleLoader - loader for bundles stored in a DeltaBundleClient instance (ReactAndroid)

All loaders derive from abstract BundleLoader class in CxxReact.

The flow of logic can be described as:

  1. Create appropriate bundle loader in RCTCxxBridge/CatalystInstance.
  2. Pass the loader and the initial bundle URL down to Instance -> BundleRegistry in CxxReact.
  3. Call Instance::runApplication (or Instance::runApplicationInRemoteDebugger when debugging - previously setSourceURl).
  4. Fetch and evaluate initial bundle in BundleRegistry.
  5. ... Fetch and evaluate additional bundles in BundleRegistry.

Notable changes

  • NativeToJsBridge is no longer a holder of RAM bundle registry. This responsibility was transferred to BundleRegistry in makeGetModuleLambda.
  • nativeRequire implementation was transferred to BundleRegistry::makeGetModuleLambda.
  • New global bundleRegistryLoad property in JS was added to allow to request bundles to be loaded either synchronously or asynchronously - check BundleRegistry::makeLoadBundleLambda.
  • registerSegment API was removed - BundleRegistry could be exposed to user space as a replacement.
  • jsQueue (MessageQueueThread) and NativeToJsBridge are stored in a BundleExecutionEnvironment struct alongside a pointer to initialBundle - this is part of the ground work to allow multiple JSContexes (as well as multiple NativeToJsBridge/MessageQueueThread). Currently there's only 1 BundleExecutionEnvironment created and it's identified in the code as default.

Example of typical use-case

A example project can be found here: https://github.com/zamotany/react-native-apennine-example

Bundler support

Multi-bundle mode is now only supported in Haul by using Webpack's DLL functionality.

Source maps support

When running in single-bundle mode, the source maps as handled as they were before and working as expected.

When running in multi-bundle mode:

  • with basic bundle, the filename in the stack trace will point to different bundles, so tools can switch between different source maps to get the original information.
  • with RAM bundles, the filename will be prefixed with bundle name for example: app0_5.js:10:5 - bundlers which want to support multi-bundle will have to account for that when creating source maps (Haul already does), so by matching the bundle name they can switching between appropriate Index Source Maps.

Missing parts

Changelog

[General] [Added] - Add BundleRegistry, bundle classes, loaders and JS API for on-demand load and evolution of additional bundles.

Test Plan

Since the changes were made to core of React Native's code, it's crucial to ensure the application can be run, not crash and display correct content. RNTester by itself can ensure that requirement.

As other functionalities, manual test were done to ensure that:

  • Chrome remote debugging still works as expected
  • Indexed and File RAM bundles are still handled properly and the app behaves as expected.
  • Requesting and loading a single and multiple bundles is working and the app behaves as expected.
  • On-demand loading and evaluation of additional bundle in release mode is working and the app behaves as expected.

cc: @matthargett @joeblynch @grabbou

@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. p: Callstack Partner: Callstack Partner labels Jul 5, 2019
@react-native-bot react-native-bot added Tech: Bundler 📦 This issue is related to the bundler (Metro, Haul, etc) used. Type: Enhancement A new feature or enhancement of an existing feature. labels Jul 5, 2019
@cpojer
Copy link
Contributor

cpojer commented Jul 9, 2019

Thank you for working on this. This is really massive. I unfortunately don't think it'll be possible to both review and land a change this large that affects many core pieces of React Native. I would suggest splitting this up into as many small pull requests as possible that can all be reviewed in isolation.

@zamotany
Copy link
Author

zamotany commented Jul 9, 2019

@cpojer
Due to the nature of the changes it will be difficult to split them apart, though it might be doable. But before, I have couple of questions:

  1. Does each small PR have to be fully integrated and used in the code?
  2. Does each small PR have to work?
  3. Can the PR add new files, which won't be used at the beginning?

Right now I have couple of ideas how to do that in mind:

  1. Split this PR into smaller ones, each one building on top of the previous and merge that when all of them are accepted.
  2. Split this PR per platform + ReactCommon, which means that after one is merged other platform won't compile, so all of those would need to be merged at once after all of them are accepted.
  3. Create many small PRs that add new files and don't break the code and one slightly bigger one which ties them together.
  4. (worst case) Split this PR into smaller ones, where each one is fully integrated and works - huge chunk of files will need to be written from scratch and then replace with the final implementation, which means that there will be a lot of throw-away code and diffs just to make things work in isolation. This is also very time consuming and will take a lot of effort.


mDevBundlesContainer = new DevBundlesContainer(bundleURL);
// DOWNLOAD INITIAL BUNDLE
downloadBundle(new DownloadListener() {
Copy link
Contributor

Choose a reason for hiding this comment

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

can the method here be renamed to downloadInitialBundle so that the comment can be removed?

ReactAndroid/src/main/jni/react/jni/JDevBundlesContainer.h Outdated Show resolved Hide resolved
@zamotany
Copy link
Author

zamotany commented Jul 15, 2019

@cpojer
Any feedback on how should we go about splitting the PR from this comment?

@cpojer
Copy link
Contributor

cpojer commented Jul 15, 2019

I think there is no agreement yet on the RN team whether we'll want to support this large change to React Native itself and whether now is a good time for this.

cc @fkgozali

NSString *const RCTFileBundleLoaderErrorDomain = @"RCTFileBundleLoaderErrorDomain";
static const int32_t JSNoBytecodeFileFormatVersion = -1;

//TODO FIGURE HOW TO THROW ERRORS HERE
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this comment need to be resolved before merging?

@cpojer
Copy link
Contributor

cpojer commented Jul 24, 2019

Please check out my comment on the RFC that goes into details on how we could move forward: react-native-community/discussions-and-proposals#127 (comment)

Based on this, I will close this PR and hope we can move forward with smaller changes.

@cpojer cpojer closed this Jul 24, 2019
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. p: Callstack Partner: Callstack Partner Tech: Bundler 📦 This issue is related to the bundler (Metro, Haul, etc) used. Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants