Bring back "Use numeric identifiers when building a bundle"

Summary:This brings back "Use numeric identifiers when building a bundle", previously backed out.
This version passes on the correct entry module name to code that decides transform options.

Original Description:
Since the combination of node and haste modules (and modules that can be required as both node and haste module) can lead to situations where it’s impossible to decide an unambiguous module identifier, this diff switches all module ids to integers. Each integer maps to an absolute path to a JS file on disk.

We also had a problem, where haste modules outside and inside node_modules could end up with the same module identifier.

This problem has not manifested yet, because the last definition of a module wins. It becomes a problem when writing file-based unbundle modules to disk: the same file might be written to concurrently, leading to invalid code.

Using indexed modules will also help indexed file unbundles, as we can encode module IDs as integers rather than scanning string IDs.

Reviewed By: martinbigio

Differential Revision: D2855202

fb-gh-sync-id: 9a011bc403690e1522b723e5742bef148a9efb52
shipit-source-id: 9a011bc403690e1522b723e5742bef148a9efb52

Libraries/Utilities/Systrace.js

@@ -202,4 +202,12 @@ var Systrace = {
 
 Systrace.setEnabled(global.__RCTProfileIsProfiling || false);
 
+if (__DEV__) {
+  // This is needed, because require callis in polyfills are not processed as
+  // other files. Therefore, calls to `require('moduleId')` are not replaced
+  // with numeric IDs
+  // TODO(davidaurelio) Scan polyfills for dependencies, too (t9759686)
+  require.Systrace = Systrace;
+}
+
 module.exports = Systrace;

local-cli/bundle/output/unbundle/as-assets.js

@@ -42,7 +42,7 @@ function saveAsAssets(bundle, options, log) {
   const writeUnbundle =
     createDir(modulesDir).then( // create the modules directory first
       Promise.all([
-        writeModules(modulesDir, modules, encoding),
+        writeModules(modules, modulesDir, encoding),
         writeFile(bundleOutput, startupCode, encoding),
         writeMagicFlagFile(modulesDir),
       ])
@@ -57,45 +57,17 @@ function createDir(dirName) {
     mkdirp(dirName, error => error ? reject(error) : resolve()));
 }
 
-function createDirectoriesForModules(modulesDir, modules) {
-  const dirNames =
-    modules.map(name => {
-      // get all needed directory names
-      const dir = path.dirname(name);
-      return dir === '.' || dir === '' ? null : path.join(modulesDir, dir);
-    })
-    .filter(Boolean) // remove empty directories
-    .sort()
-    .filter((dir, i, dirs) => {
-      // remove parent directories and dedupe.
-      // After sorting, parent directories are located before child directories
-      const next = dirs[i + 1];
-      return !next || next !== dir && !next.startsWith(dir + path.sep);
-    });
-
-  return dirNames.reduce(
-    (promise, dirName) =>
-      promise.then(() => createDir(dirName)), Promise.resolve());
-}
-
 function writeModuleFile(module, modulesDir, encoding) {
-  const {name, code} = module;
-  return writeFile(path.join(modulesDir, name + '.js'), code, encoding);
+  const {code, id} = module;
+  return writeFile(path.join(modulesDir, id + '.js'), code, encoding);
 }
 
-function writeModuleFiles(modules, modulesDir, encoding) {
+function writeModules(modules, modulesDir, encoding) {
   const writeFiles =
     modules.map(module => writeModuleFile(module, modulesDir, encoding));
   return Promise.all(writeFiles);
 }
 
-function writeModules(modulesDir, modules, encoding) {
-  return (
-    createDirectoriesForModules(modulesDir, modules.map(({name}) => name))
-      .then(() => writeModuleFiles(modules, modulesDir, encoding))
-  );
-}
-
 function writeMagicFlagFile(outputDir) {
   /* global Buffer: true */
   const buffer = Buffer(4);

local-cli/server/util/attachHMRServer.js

@@ -39,6 +39,8 @@ function attachHMRServer({httpServer, path, packagerServer}) {
       hot: true,
       entryFile: bundleEntry,
     }).then(response => {
+      const {getModuleId} = response;
+
       // for each dependency builds the object:
       // `{path: '/a/b/c.js', deps: ['modA', 'modB', ...]}`
       return Promise.all(Object.values(response.dependencies).map(dep => {
@@ -76,30 +78,25 @@ function attachHMRServer({httpServer, path, packagerServer}) {
         // map from module name to the modules' dependencies the bundle entry
         // has
         const dependenciesModulesCache = Object.create(null);
-        return Promise.all(response.dependencies.map(dep => {
-          return dep.getName().then(depName => {
-            dependenciesModulesCache[depName] = dep;
-          });
-        })).then(() => {
-          const inverseDependencies = getInverseDependencies(response);
-          const inverseDependenciesCache = Object.create(null);
-          return Promise.all(
-            Array.from(inverseDependencies).map(([module, dependents]) => {
-              return Promise.all([
-                module.getName(),
-                Promise.all(Array.from(dependents).map(m => m.getName())),
-              ]).then(([moduleName, dependentsNames]) => {
-                inverseDependenciesCache[moduleName] = dependentsNames;
-              });
-            })
-          ).then(() => ({
-            dependenciesCache,
-            dependenciesModulesCache,
-            shallowDependencies,
-            inverseDependenciesCache,
-            resolutionResponse: response,
-          }));
+        response.dependencies.forEach(dep => {
+          dependenciesModulesCache[getModuleId(dep)] = dep;
         });
+
+
+        const inverseDependenciesCache = Object.create(null);
+        const inverseDependencies = getInverseDependencies(response);
+        for (const [module, dependents] of inverseDependencies) {
+          inverseDependenciesCache[getModuleId(module)] =
+            Array.from(dependents).map(getModuleId);
+        }
+
+        return {
+          dependenciesCache,
+          dependenciesModulesCache,
+          shallowDependencies,
+          inverseDependenciesCache,
+          resolutionResponse: response,
+        };
       });
     });
   }

packager/package.json

@@ -1,5 +1,5 @@
 {
-  "version": "0.2.0",
+  "version": "0.3.0",
   "name": "react-native-packager",
   "description": "Build native apps with React!",
   "repository": {

packager/react-packager/src/Bundler/Bundle.js

@@ -37,7 +37,6 @@ class Bundle extends BundleBase {
       map: moduleTransport.map,
       meta: moduleTransport.meta,
       minify: this._minify,
-      polyfill: module.isPolyfill(),
     }).then(({code, map}) => {
       // If we get a map from the transformer we'll switch to a mode
       // were we're combining the source maps as opposed to
@@ -65,10 +64,11 @@ class Bundle extends BundleBase {
   }
 
   _addRequireCall(moduleId) {
-    const code = ';require("' + moduleId + '");';
+    const code = `;require(${JSON.stringify(moduleId)});`;
     const name = 'require-' + moduleId;
     super.addModule(new ModuleTransport({
       name,
+      id: this._numRequireCalls - 1,
       code,
       virtual: true,
       sourceCode: code,
@@ -118,6 +118,7 @@ class Bundle extends BundleBase {
       modules: modules.map(({name, code, polyfill}) =>
         ({name, code, polyfill})
       ),
+      modules,
     };
   }
 

packager/react-packager/src/Bundler/HMRBundle.js

@@ -21,16 +21,17 @@ class HMRBundle extends BundleBase {
   }
 
   addModule(resolver, response, module, moduleTransport) {
-    return resolver.resolveRequires(
+    const code = resolver.resolveRequires(
       response,
       module,
       moduleTransport.code,
       moduleTransport.meta.dependencyOffsets,
-    ).then(code => {
-      super.addModule(new ModuleTransport({...moduleTransport, code}));
-      this._sourceMappingURLs.push(this._sourceMappingURLFn(moduleTransport.sourcePath));
-      this._sourceURLs.push(this._sourceURLFn(moduleTransport.sourcePath));
-    });
+    );
+
+    super.addModule(new ModuleTransport({...moduleTransport, code}));
+    this._sourceMappingURLs.push(this._sourceMappingURLFn(moduleTransport.sourcePath));
+    this._sourceURLs.push(this._sourceURLFn(moduleTransport.sourcePath));
+    return Promise.resolve();
   }
 
   getModulesNamesAndCode() {

packager/react-packager/src/Bundler/__tests__/Bundle-test.js

@@ -394,6 +394,7 @@ function addModule({bundle, code, sourceCode, sourcePath, map, virtual, polyfill
 function createModuleTransport(data) {
   return new ModuleTransport({
     code: '',
+    id: '',
     sourceCode: '',
     sourcePath: '',
     ...data,

packager/react-packager/src/Bundler/index.js

@@ -106,6 +106,8 @@ class Bundler {
       mtime,
     ];
 
+    this._getModuleId = createModuleIdFactory();
+
     if (opts.transformModulePath) {
       const transformer = require(opts.transformModulePath);
       if (typeof transformer.cacheKey !== 'undefined') {
@@ -131,6 +133,7 @@ class Bundler {
       fileWatcher: opts.fileWatcher,
       assetExts: opts.assetExts,
       cache: this._cache,
+      getModuleId: this._getModuleId,
       transformCode:
         (module, code, options) =>
           this._transformer.transformFile(module.path, code, options),
@@ -208,6 +211,7 @@ class Bundler {
 
   hmrBundle(options, host, port) {
     return this._bundle({
+      ...options,
       bundle: new HMRBundle({
         sourceURLFn: this._sourceHMRURL.bind(this, options.platform, host, port),
         sourceMappingURLFn: this._sourceMappingHMRURL.bind(
@@ -216,7 +220,7 @@ class Bundler {
         ),
       }),
       hot: true,
-      ...options,
+      dev: true,
     });
   }
 
@@ -234,8 +238,18 @@ class Bundler {
     entryModuleOnly,
     resolutionResponse,
   }) {
+    if (dev && runBeforeMainModule) { // no runBeforeMainModule for hmr bundles
+      // `require` calls in the require polyfill itself are not extracted and
+      // replaced with numeric module IDs, but the require polyfill
+      // needs Systrace.
+      // Therefore, we include the Systrace module before the main module, and
+      // it will set itself as property on the require function.
+      // TODO(davidaurelio) Scan polyfills for dependencies, too (t9759686)
+      runBeforeMainModule = runBeforeMainModule.concat(['Systrace']);
+    }
+
     const onResolutionResponse = response => {
-      bundle.setMainModuleId(response.mainModuleId);
+      bundle.setMainModuleId(this._getModuleId(getMainModule(response)));
       if (bundle.setNumPrependedModules) {
         bundle.setNumPrependedModules(
           response.numPrependedDependencies + moduleSystemDeps.length
@@ -249,13 +263,23 @@ class Bundler {
         response.dependencies = moduleSystemDeps.concat(response.dependencies);
       }
     };
-    const finalizeBundle = ({bundle, transformedModules, response}) =>
+    const finalizeBundle = ({bundle, transformedModules, response, modulesByName}) =>
       Promise.all(
         transformedModules.map(({module, transformed}) =>
           bundle.addModule(this._resolver, response, module, transformed)
         )
       ).then(() => {
-        bundle.finalize({runBeforeMainModule, runMainModule});
+        const runBeforeMainModuleIds = Array.isArray(runBeforeMainModule)
+          ? runBeforeMainModule
+              .map(name => modulesByName[name])
+              .filter(Boolean)
+              .map(this._getModuleId, this)
+          : undefined;
+
+        bundle.finalize({
+          runMainModule,
+          runBeforeMainModule: runBeforeMainModuleIds,
+        });
         return bundle;
       });
 
@@ -325,6 +349,7 @@ class Bundler {
     finalizeBundle = noop,
   }) {
     const findEventId = Activity.startEvent('find dependencies');
+    const modulesByName = Object.create(null);
 
     if (!resolutionResponse) {
       let onProgess;
@@ -360,6 +385,7 @@ class Bundler {
           bundle,
           transformOptions: response.transformOptions,
         }).then(transformed => {
+          modulesByName[transformed.name] = module;
           onModuleTransformed({
             module,
             response,
@@ -371,9 +397,9 @@ class Bundler {
 
       return Promise.all(response.dependencies.map(toModuleTransport))
         .then(transformedModules =>
-          Promise
-            .resolve(finalizeBundle({bundle, transformedModules, response}))
-            .then(() => bundle)
+          Promise.resolve(
+            finalizeBundle({bundle, transformedModules, response, modulesByName})
+          ).then(() => bundle)
         );
     });
   }
@@ -481,6 +507,7 @@ class Bundler {
       [name, {code, dependencies, dependencyOffsets, map, source}]
     ) => new ModuleTransport({
       name,
+      id: this._getModuleId(module),
       code,
       map,
       meta: {dependencies, dependencyOffsets},
@@ -513,6 +540,7 @@ class Bundler {
 
       return new ModuleTransport({
         name: id,
+        id: this._getModuleId(module),
         code: code,
         sourceCode: code,
         sourcePath: module.path,
@@ -578,8 +606,9 @@ class Bundler {
       bundle.addAsset(asset);
       return new ModuleTransport({
         name,
+        id: this._getModuleId(module),
         code,
-        meta,
+        meta: meta,
         sourceCode: code,
         sourcePath: module.path,
         virtual: true,
@@ -614,4 +643,20 @@ function verifyRootExists(root) {
   assert(fs.statSync(root).isDirectory(), 'Root has to be a valid directory');
 }
 
+function createModuleIdFactory() {
+  const fileToIdMap = Object.create(null);
+  let nextId = 0;
+  return ({path}) => {
+    if (!(path in fileToIdMap)) {
+      fileToIdMap[path] = nextId;
+      nextId += 1;
+    }
+    return fileToIdMap[path];
+  };
+}
+
+function getMainModule({dependencies, numPrependedDependencies = 0}) {
+  return dependencies[numPrependedDependencies];
+}
+
 module.exports = Bundler;