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

[Discuss]Problems about unstable numeric module ID. #6

Closed
mc-zone opened this issue Jun 9, 2017 · 8 comments
Closed

[Discuss]Problems about unstable numeric module ID. #6

mc-zone opened this issue Jun 9, 2017 · 8 comments

Comments

@mc-zone
Copy link

mc-zone commented Jun 9, 2017

Do you want to request a feature or report a bug?
Feature, or discuss first.

What is the current behavior?

Modules are now packed by unstable numeric module ID. There are some bad parts of this behavior:

1. Unfriendly to diff.

Supposing there is an entry which some deps like following:

import a from "./lib/a";
import b from "./lib/b";
import c from "./lib/c";
import d from "./lib/d";

Bundle it first. Then remove one of deps, for example, ./lib/a:

-import a from "./lib/a";
import b from "./lib/b";
import c from "./lib/c";
import d from "./lib/d";

Bundle it again. Check bundle's diff:

image

As you see, all define/require code (module ID, exactly) about deps after ./lib/a were changed.

This may give rise to lots of unecessary mutated in some projects's bundle which has huge dependency graph. It's not friendly to bundle diff/update. (Some Apps and 3rd. tools will update bundle by diff algorithm to reduce the consumption from un-splitting bundle size.)

2. Can' t run muti-bundle(RN App) at one context.

In our team, there are many native Apps which have been integrated React Native paritially, and each native App may include mutiple RN Apps (with mutiple entries).

We are finding a way that can run mutiple RN Apps in a common JS context so that we don't need to recreate bridge/context, run bundle and define method/modules over and over, even can share data and modules with each other.

Think about the style of Node.js server application, we don't need to redefine modules like react, react-dom, react-router blah-blah for each route when it called by requests.

image

But at least now, It can't be implemented because of numeric module ID. Bundles will override other's modules which always start with 0, 1, 2 but not an unique ID.

3. Unfriendly to bundle-splitting (module reference).

We are also trying to split bundle for decrease update download size. You can see an earlier incomplete PR facebook/react-native#10804 I've sent.

We splited most of library modules, like react, react-native and our common utils into a base bundle, only pack business code into business bundle, then maintaining a manifest.json file to handle module call from business bundle to base bundle (Like webpack's DLL way).

image

But you can guess that numeric module ID also make it unstable. If we add a module, or reorder the imports to any of deps in base bundle, manifest will get a huge change. We have to repack all business bundles. This means that we can't make a stable module reference by current packager.

If the current behavior is a bug, please provide the steps to reproduce and a minimal repository on GitHub that we can yarn install and yarn test.
Not a bug.

What is the expected behavior?

Could metro-bundler support string module ID?

Support string module ID so that we can get more stable bundle, and more friendly to diff, make stable reference and work on bundle-splitting.

We know that each module has an unique path relative to project's root path. Maybe we can create a hash by the path in moduleIdFactory, to get an unique stable ID.

And if we want to create a bundle with isolate IDs (run different projects bundle, or upgrade module partially), we also can add a prefix to the moduleIdFactory when pack the bundle. It's absolutely more extensible than numeric.

I also noticed about the indexed-ram-bundle in unbundle way. It has relied on numeric ID strongly AFAIK. Could metro-bundler make compatible with this two ways? Or through option to enable stable string module ID?

looking forward to your opinions @cpojer @jeanlauliac @amasad @davidaurelio. And willing to send PR if needed!

@davidaurelio
Copy link
Contributor

davidaurelio commented Jun 9, 2017

the new ModuleGraph implementation only needs module IDs at the very end of the build process.

We could count the modules in each chunk, and assign them ranges, e.g. 0–1000 for the main bundle, 1000–2000 for the first chunk, depending on the number of files in each chunk.

By adding some headroom we could minimize changes to chunks that should normally be unaffected. It’s a 20%/80% solution, i.e. not perfect, but maybe good enough.

Could metro-bundler support string module ID?

numbers allow for some interesting optimizations, specifically memory-friendly look-up tables. We use that for Random Access Bundles (“unbundle”, yet to be documented :-( ) in native code (C++).

Unique hashes have to have a certain length to avoid collisions, and for large bundles that can make a non-trivial difference in size.

Everything is a tradeoff, we have to find the right one.

Using hashes in Random Access Bundles means we would have to change lookup structures from tables to hash maps, and that is undesirable, as it would incur a small TTI penalty.

Currently we are focused on runtime performance for mobile. Stable module IDs are desirable, too, but we don’t want the latter at the expense of the former.

We want to address the “multiple app” scenario in React Native with clever bundle splitting that allows sharing code between chunks, while retaining the positive properties of numeric module IDs.

What you can do as of today is to implement the postProcessModulesForBuck hook to order modules by path. This should have neutral performance for plain text bundles, but creates significant TTI regressions when using a Random Access Bundle.

@mc-zone
Copy link
Author

mc-zone commented Jun 9, 2017

Thanks for your reply! @davidaurelio

Yes, I probably knew we should retain numeric module ID for Random Access Bundle because of it's special index table. Now your reply also help me realize more about it.

How about when using normal bundle, except unbundle, provide an option to let users choose, or define their own moduleIdFactory?

I can realize about bundle size increase that your've mentioned. But I think it maybe not so much.

In fact, our team are using a local fork of react-native based on 0.35, changed the createModuleIdFactory to use hash module ID. code like:

function createModuleIdFactory({projectRoots, prefix}) {
  const fileToIdMap = Object.create(null);
  const usedIds = {};
  return ({path: modulePath, name}) => {
    var relativePath = getPathRelativeToRoot(projectRoots, modulePath);
    if (!(modulePath in fileToIdMap)) {
      fileToIdMap[modulePath] = getModuleHashedPathId(relativePath, usedIds);
    }
    return fileToIdMap[modulePath];
  };
}
function getModuleHashedPathId(path, usedIds){
  var len = 4;
  var hash = crypto.createHash("md5");
  hash.update(path);
  var id = hash.digest("base64");
  while(usedIds[id.substr(0, len)]){
    len++;
  }
  id = id.substr(0, len);
  usedIds[id] = path;
  return id;
}

Part of the output bundle like this:

image

Now I've make a compare at one of our RN apps, which total 539 modules in production. Here is the size change betwteen numeric module ID and hash module ID (both with uglify, dev=false):

image
838KB - 847KB. only 10KB increased.

So in contrast, I prefer to use hash module ID for normal text bundle, it give us ability to split bundle by stable module ID.

What you can do as of today is to implement the postProcessModulesForBuck hook to order modules by path.

I haven't tried postProcessModulesForBuck yet. I will have a look at it. But I think maybe reorder modules by path can't keep stable when deps was added/removed, their are still numbers related.

If I have any mistake please correct me. Thanks again!

@zimo888
Copy link

zimo888 commented Aug 9, 2017

I agree with @mc-zone 's opinion, We have a B2B project that likes the expo, developers upload their react-native code to service, after that, service sends developer's code to our customer's app and combine it.

We have to split the bundle file to a base bundle and developer's business bundles, If we use numeric module ID, because the base module has only one,Two different developer's generate there module ID will be same.
Only use moduleName or hashName can do these.
I Sincerely hope the bundle commands of the packager will be more flexible and more powerful,Thank you very much.

@lishoulong
Copy link

@mc-zone ,I use your createModuleIdFactory for hashed ids, but when I run "react-native run build --dev false ...", it error the followings "Unexpected token: punc ()) in file "/Users/lifeifei/Documents/react-native/sourceCode/newRNBundle/index.js" at line 2:26".

I guess this is the cause of --dev false, maybe uglify.

I only change the code for the createModuleIdFactory as yours, Is I use right?

thanks!

@mc-zone
Copy link
Author

mc-zone commented Nov 24, 2017

@lishoulong I'm not sure, haven't seen this error. Could you provide a repo about your code?

@lishoulong
Copy link

Hi,
I fork the official metro-bundler, and I change the method of createModuleIdFactory as of you mention above.

The blow is my commits, please take your time have a look.

lishoulong@8c9f3bd

@lishoulong
Copy link

@mc-zone , I know what my question is , the below is the part code I copy from the builded bundle.

var AnimatedAddition = require(helpcenter-AnimatedAddition); // helpcenter-AnimatedAddition = ./nodes/AnimatedAddition;

As you can see the required helpcenter-AnimatedAddition is not quoted , so the program look helpcenter-AnimatedAddition as a variable.The question is some require content is also quoted.

But I do not know where the problem is?

@rafeca
Copy link
Contributor

rafeca commented Jan 1, 2018

Hey @mc-zone, @lishoulong : We recently merged f347e4f, which allows to customize the createModuleIdFactory() function, so if you use v0.24.1 you're going to be able to use your custom logic for generating the module ids.

@rafeca rafeca closed this as completed Jan 1, 2018
krmao added a commit to krmao/smart-metro that referenced this issue Jun 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants