Skip to content

Conversation

@mc-zone
Copy link

@mc-zone mc-zone commented Nov 8, 2016

Thanks for submitting a pull request! Please provide enough information so that others can review your pull request:

Unless you are a React Native release maintainer and cherry-picking an existing commit into a current release, ensure your pull request is targeting the master React Native branch.

Target: master

Explain the motivation for making this change. What existing problem does the pull request solve?

Motivation

Add bundle-splitting support.

If we could split some initial and definition codes to a base.jsbundle and output it's module lists (manifest file), then we use it as a reference to bundle business.bundle and others. It can:

  • Decrease initial time (Run base bundle previously).
  • Update bundle partially (Fetch small size business bundle from CDN).
  • Share common modules between multiple bundle (Imagine we have more than one business bundle).

There is a sketch map:
image
Detail motivation and explain can see #5399.

Prefer small pull requests. These are much easier to review and more likely to get merged. Make sure the PR does only one thing, otherwise please split it.

Small pull requests

This PR only include bundle-splitting support in Packager part (manifest output, manifest reference).

PS: Next steps we can try and add Native API to run bundle separately (part with runApplication), enhance muti bundle use case (just like moduleId prefix), and others. Hopefully.

Test plan (required)

Test plan

I've added tests for these points ( in Bundler-test.js、Bundle-test.js):

When we build a bundle:

  • If given manifest output, output the manifest file that include moudle lists and last moduleId.

  • If given manifest file, should skip dependencies that already exists in the manifest on resolutionResponse in bundleBundle.

  • If require the module that has exists in given manifest file, should use it existing moduleId (Should read reference in getModuleId).

  • If given manifest file, the new moduleIds of modules in this bundle should not duplicate with reference bundle (greater than the biggest/last id record in the manifest file).

  • Don't insert prelude and polyfills repeatly if has provide manifest file when bundle (There are only in the base.bundle).

Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI.

Example

  • If provide the --manifest-output args in bundle cli, should output a manifest file write to the given path that include the module list info of bundle.

    Useage:
    react-native bundle --entry-file ./base.android.js --platform android --bundle-output ./output/base.android.bundle --manifest-output ./output/base.android.manifest.json

    And the output:
    image

  • if provide the --manifest-file args in bundle cli, should read the given manifest file and use it to skip the modules which already exists in manifest module list, on resolutionResponse in buildBundle.

    Useage:
    react-native bundle --entry-file ./index.android.js --platform android --bundle-output ./output/index.android.bundle --manifest-file ./output/base.android.manifest.json

    And the output the index.android.bundle:
    image
    It contains modules that are not exist in the manifest file.


Make sure tests pass on both Travis and Circle CI.

Code formatting

Look around. Match the style of the rest of the codebase. See also the simple style guide.

For more info, see the "Pull Requests" section of our "Contributing" guidelines.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @davidaurelio and @ide to be potential reviewers.

@mc-zone mc-zone changed the title [Packager] Add bundle-splitting support by dep/modules output and reference in buildBundle [Packager] Add bundle-splitting support by dep/modules info output and reference in buildBundle Nov 8, 2016
@mc-zone mc-zone changed the title [Packager] Add bundle-splitting support by dep/modules info output and reference in buildBundle [Packager] Add bundle-splitting support by dep/modules info output and reference in build bundle Nov 8, 2016
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 8, 2016
@hramos
Copy link
Contributor

hramos commented Nov 11, 2016

Thanks for the PR. Can you take care of the conflicts first?

Support output a manifest file to record bundle's modules when
build a bundle.

Support build bundle with a manifest file to skip existing modules.

Add tests.
@mc-zone mc-zone closed this Nov 12, 2016
@mc-zone mc-zone reopened this Nov 12, 2016
@mc-zone
Copy link
Author

mc-zone commented Nov 12, 2016

@hramos Sorry. I didn't see it. I've merge done, repair errors, and squash my commits now.

But currently the Circle CI can't be finished:

>exceeded the memory limit of 4G on 1 container.

I don't clear about that, how should I retry/fix it?

It's OK now, tests have passed after I click Update branch button and retry.

@yanweimin7
Copy link

@mc-zone where is the nativeCode, like runJsBundleInContext

@mc-zone
Copy link
Author

mc-zone commented Nov 13, 2016

@mc-zone where is the nativeCode, like runJsBundleInContext

Will be provided in another PR. This PR only include packager code first.

@mkonicek
Copy link
Contributor

Thanks for explaining the motivation. We might be working on different solutions to the same problems but I'm not sure.

I'll let @cpojer take a look.

swainet added a commit to react-component/rn-packager that referenced this pull request Dec 1, 2016
@cpojer
Copy link
Contributor

cpojer commented Dec 1, 2016

Hey! I'm sorry, we are a bit swamped right now on RNP but this is on top of our mind and we'll get back to you about it.

@yiminghe
Copy link
Contributor

yiminghe commented Dec 2, 2016

can you give a option to not use id as module name (it's easy to change as framework change, but business bundle does not need to rebundle )

@ylquankai
Copy link

How is it going?

@mc-zone
Copy link
Author

mc-zone commented Dec 25, 2016

@yiminghe Incrementing number to module ID is not stable , it is a certainly problem when using muti-bundle and dependencies reference. But directly use module name is also not good enough (It was used in earlier versions, I remember). I think we need a better solution about it, for example, maybe name-hash ID (by module path) is better.

If this bundle-splitting solution can be accepted and merged, I would try to take another PR to resolve this ASAP.

@njafei
Copy link

njafei commented Feb 4, 2017

android can try unbundle. this will separate mian.jsbundle into pieces of js files

@swainet
Copy link

swainet commented Feb 14, 2017

Any progress?

@markzhai
Copy link

markzhai commented Mar 9, 2017

Any updates about this PR ?

@cpojer
Copy link
Contributor

cpojer commented May 2, 2017

We are considering this for H2 2017.

@tanpuer
Copy link

tanpuer commented May 31, 2017

@mc-zone
Amazing!
Is there any demo for reference or can you share how much init time can be saved?

@cpojer
Copy link
Contributor

cpojer commented Jun 8, 2017

Metro Bundler is now in a separate repository. Would you mind sending the same pull request to the metro-bundler repo? https://github.com/facebook/metro-bundler We are finally able to take these things on more easily.

Thank you!

@cpojer cpojer closed this Jun 8, 2017
@mc-zone
Copy link
Author

mc-zone commented Jun 8, 2017

Sure. I will try.

@jxiang112
Copy link

@mc-zone where is the nativeCode, like runJsBundleInContext
Will be provided in another PR. This PR only include packager code first.

has provided? and where?

@ArunKulasegaran
Copy link

Any update on the native changes or do we have any sample project?
Please help

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. JavaScript

Projects

None yet

Development

Successfully merging this pull request may close these issues.