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

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": {

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,
     };
   }
 

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));
+    super.addModule(new ModuleTransport({...moduleTransport, code}));
-      this._sourceURLs.push(this._sourceURLFn(moduleTransport.sourcePath));
+    this._sourceMappingURLs.push(this._sourceMappingURLFn(moduleTransport.sourcePath));
-    });
+    this._sourceURLs.push(this._sourceURLFn(moduleTransport.sourcePath));
+    return Promise.resolve();
   }
 
   getModulesNamesAndCode() {

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,

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
+          Promise.resolve(
-            .resolve(finalizeBundle({bundle, transformedModules, response}))
+            finalizeBundle({bundle, transformedModules, response, modulesByName})
-            .then(() => bundle)
+          ).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;

react-packager/src/Resolver/__tests__/Resolver-test.js

@@ -67,6 +67,7 @@ describe('Resolver', function() {
 
   function createModule(id, dependencies) {
     var module = new Module({});
+    module.path = id;
     module.getName.mockImpl(() => Promise.resolve(id));
     module.getDependencies.mockImpl(() => Promise.resolve(dependencies));
     return module;
@@ -109,6 +110,7 @@ describe('Resolver', function() {
       var deps = [module];
 
       var depResolver = new Resolver({
+        getModuleId: createGetModuleId(),
         projectRoot: '/root',
       });
 
@@ -262,11 +264,17 @@ describe('Resolver', function() {
   });
 
   describe('wrapModule', function() {
-    pit('should resolve modules', function() {
+    let depResolver, getModuleId;
-      var depResolver = new Resolver({
+    beforeEach(() => {
+      getModuleId = createGetModuleId();
+      depResolver = new Resolver({
+        depResolver,
+        getModuleId,
         projectRoot: '/root',
       });
+    });
 
+    pit('should resolve modules', function() {
       /*eslint-disable */
       var code = [
         // require
@@ -300,18 +308,24 @@ describe('Resolver', function() {
         ];
       }
 
+      const moduleIds = new Map(
+        resolutionResponse
+          .getResolvedDependencyPairs()
+          .map(([importId, module]) => [importId, getModuleId(module)])
+      );
+
       return depResolver.wrapModule({
         resolutionResponse,
-        module: createModule('test module', ['x', 'y']),
+        module: module,
         name: 'test module',
         code,
         meta: {dependencyOffsets}
       }).then(({code: processedCode}) => {
         expect(processedCode).toEqual([
-          '__d("test module", function(global, require, module, exports) {' +
+          `__d(${getModuleId(module)} /* test module */, function(global, require, module, exports) {` +
           // require
-          'require("changed")',
+          `require(${moduleIds.get('x')} /* x */)`,
-          'require("Y")',
+          `require(${moduleIds.get('y')} /* y */)`,
           'require( \'z\' )',
           'require( "a")',
           'require("b" )',
@@ -327,7 +341,7 @@ describe('Resolver', function() {
         mainModuleId: 'test module',
       });
       const inputMap = {version: 3, mappings: 'ARBITRARY'};
-      return new Resolver({projectRoot: '/root'}).wrapModule({
+      return depResolver.wrapModule({
         resolutionResponse,
         module,
         name: 'test module',
@@ -362,7 +376,7 @@ describe('Resolver', function() {
       let depResolver, module, resolutionResponse;
 
       beforeEach(() => {
-        depResolver = new Resolver({projectRoot: '/root'});
+        depResolver = new Resolver({getModuleId, projectRoot: '/root'});
         module = createJsonModule(id);
         resolutionResponse = new ResolutionResponseMock({
           dependencies: [module],
@@ -375,7 +389,7 @@ describe('Resolver', function() {
           .wrapModule({resolutionResponse, module, name: id, code})
           .then(({code: processedCode}) =>
             expect(processedCode).toEqual([
-              `__d(${JSON.stringify(id)}, function(global, require, module, exports) {`,
+              `__d(${getModuleId(module)} /* ${id} */, function(global, require, module, exports) {`,
               `module.exports = ${code}\n});`,
             ].join('')));
       });
@@ -391,6 +405,7 @@ describe('Resolver', function() {
           Promise.resolve({code, map}));
         depResolver = new Resolver({
           projectRoot: '/root',
+          getModuleId,
           minifyCode
         });
         module = createModule(id);
@@ -403,7 +418,7 @@ describe('Resolver', function() {
       });
 
       pit('should invoke the minifier with the wrapped code', () => {
-        const wrappedCode = `__d("${id}", function(global, require, module, exports) {${code}\n});`
+        const wrappedCode = `__d(${getModuleId(module)} /* ${id} */, function(global, require, module, exports) {${code}\n});`
         return depResolver
           .wrapModule({
             resolutionResponse,
@@ -430,4 +445,17 @@ describe('Resolver', function() {
       });
     });
   });
+
+  function createGetModuleId() {
+    let nextId = 1;
+    const knownIds = new Map();
+    function createId(path) {
+      const id = nextId;
+      nextId += 1;
+      knownIds.set(path, id);
+      return id;
+    }
+
+    return ({path}) => knownIds.get(path) || createId(path);
+  }
 });

react-packager/src/Resolver/index.js

@@ -47,6 +47,10 @@ const validateOpts = declareOpts({
     type: 'object',
     required: true,
   },
+  getModuleId: {
+    type: 'function',
+    required: true,
+  },
   transformCode: {
     type: 'function',
   },
@@ -103,8 +107,10 @@ class Resolver {
       cache: opts.cache,
       shouldThrowOnUnresolvedErrors: (_, platform) => platform === 'ios',
       transformCode: opts.transformCode,
+      assetDependencies: ['AssetRegistry'],
     });
 
+    this._getModuleId = options.getModuleId;
     this._minifyCode = opts.minifyCode;
     this._polyfillModuleNames = opts.polyfillModuleNames || [];
 
@@ -139,6 +145,8 @@ class Resolver {
         polyfill => resolutionResponse.prependDependency(polyfill)
       );
 
+      // currently used by HMR
+      resolutionResponse.getModuleId = this._getModuleId;
       return resolutionResponse.finalize();
     });
   }
@@ -186,53 +194,40 @@ class Resolver {
   }
 
   resolveRequires(resolutionResponse, module, code, dependencyOffsets = []) {
-    return Promise.resolve().then(() => {
+    const resolvedDeps = Object.create(null);
-      const resolvedDeps = Object.create(null);
-      const resolvedDepsArr = [];
 
-      return Promise.all(
+    // here, we build a map of all require strings (relative and absolute)
-        // here, we build a map of all require strings (relative and absolute)
+    // to the canonical ID of the module they reference
-        // to the canonical name of the module they reference
+    resolutionResponse.getResolvedDependencyPairs(module)
-        resolutionResponse.getResolvedDependencyPairs(module).map(
+      .forEach(([depName, depModule]) => {
-          ([depName, depModule]) => {
+        if (depModule) {
-            if (depModule) {
+          resolvedDeps[depName] = this._getModuleId(depModule);
-              return depModule.getName().then(name => {
+        }
-                resolvedDeps[depName] = name;
-                resolvedDepsArr.push(name);
-              });
-            }
-          }
-        )
-      ).then(() => {
-        const relativizeCode = (codeMatch, quot, depName) => {
-          // if we have a canonical name for the module imported here,
-          // we use it, so that require() is always called with the same
-          // id for every module.
-          // Example:
-          // -- in a/b.js:
-          //    require('./c') => require('a/c');
-          // -- in b/index.js:
-          //    require('../a/c') => require('a/c');
-          const depId = resolvedDeps[depName];
-          if (depId) {
-            return quot + depId + quot;
-          } else {
-            return codeMatch;
-          }
-        };
-
-        code = dependencyOffsets.reduceRight((codeBits, offset) => {
-          const first = codeBits.shift();
-          codeBits.unshift(
-            first.slice(0, offset),
-            first.slice(offset).replace(/(['"])([^'"']*)\1/, relativizeCode),
-          );
-          return codeBits;
-        }, [code]);
-
-        return code.join('');
       });
-    });
+
+    // if we have a canonical ID for the module imported here,
+    // we use it, so that require() is always called with the same
+    // id for every module.
+    // Example:
+    // -- in a/b.js:
+    //    require('./c') => require(3);
+    // -- in b/index.js:
+    //    require('../a/c') => require(3);
+    const replaceModuleId = (codeMatch, quote, depName) =>
+      depName in resolvedDeps
+        ? `${JSON.stringify(resolvedDeps[depName])} /* ${depName} */`
+        : codeMatch;
+
+    code = dependencyOffsets.reduceRight((codeBits, offset) => {
+      const first = codeBits.shift();
+      codeBits.unshift(
+        first.slice(0, offset),
+        first.slice(offset).replace(/(['"])([^'"']*)\1/, replaceModuleId),
+      );
+      return codeBits;
+    }, [code]);
+
+    return code.join('');
   }
 
   wrapModule({
@@ -247,18 +242,24 @@ class Resolver {
     if (module.isJSON()) {
       code = `module.exports = ${code}`;
     }
-    const result = module.isPolyfill()
+
-      ? Promise.resolve({code: definePolyfillCode(code)})
+    if (module.isPolyfill()) {
-      : this.resolveRequires(
+      code = definePolyfillCode(code);
+    } else {
+      const moduleId = this._getModuleId(module);
+      code = this.resolveRequires(
         resolutionResponse,
         module,
         code,
         meta.dependencyOffsets
-      ).then(code => ({code: defineModuleCode(name, code), map}));
+      );
+      code = defineModuleCode(moduleId, code, name);
+    }
+
 
     return minify
-      ? result.then(({code, map}) => this._minifyCode(module.path, code, map))
+      ? this._minifyCode(module.path, code, map)
-      : result;
+      : Promise.resolve({code, map});
   }
 
   minifyModule({path, code, map}) {
@@ -270,10 +271,10 @@ class Resolver {
   }
 }
 
-function defineModuleCode(moduleName, code) {
+function defineModuleCode(moduleName, code, verboseName = '') {
   return [
     `__d(`,
-    `${JSON.stringify(moduleName)}, `,
+    `${JSON.stringify(moduleName)} /* ${verboseName} */, `,
     `function(global, require, module, exports) {`,
       `${code}`,
     '\n});',

react-packager/src/Resolver/polyfills/require-unbundle.js

@@ -54,7 +54,9 @@ function loadModuleImplementation(moduleId, module) {
   // The systrace module will expose itself on the require function so that
   // it can be used here.
   // TODO(davidaurelio) Scan polyfills for dependencies, too (t9759686)
-  const {Systrace} = require;
+  if (__DEV__) {
+    var {Systrace} = require;
+  }
 
   const exports = module.exports = {};
   const {factory} = module;

react-packager/src/Resolver/polyfills/require.js

@@ -59,18 +59,31 @@ function requireImpl(id) {
     );
   }
 
+  // `require` calls int  the require polyfill itself are not analyzed and
+  // replaced so that they use numeric module IDs.
+  // The systrace module will expose itself on the require function so that
+  // it can be used here.
+  // TODO(davidaurelio) Scan polyfills for dependencies, too (t9759686)
+  if (__DEV__) {
+    var {Systrace} = require;
+  }
+
   try {
     // We must optimistically mark mod as initialized before running the factory to keep any
     // require cycles inside the factory from causing an infinite require loop.
     mod.isInitialized = true;
 
-    __DEV__ && Systrace().beginEvent('JS_require_' + id);
+    if (__DEV__) {
+      Systrace.beginEvent('JS_require_' + id);
+    }
 
     // keep args in sync with with defineModuleCode in
     // packager/react-packager/src/Resolver/index.js
     mod.factory.call(global, global, require, mod.module, mod.module.exports);
 
-    __DEV__ && Systrace().endEvent();
+    if (__DEV__) {
+      Systrace.endEvent();
+    }
   } catch (e) {
     mod.hasError = true;
     mod.isInitialized = false;
@@ -80,15 +93,9 @@ function requireImpl(id) {
   return mod.module.exports;
 }
 
-const Systrace = __DEV__ && (() => {
+if (__DEV__) {
-  var _Systrace;
+  require.Systrace = { beginEvent: () => {}, endEvent: () => {} };
-  try {
+}
-    _Systrace = require('Systrace');
-  } catch (e) {}
-
-  return _Systrace && _Systrace.beginEvent ?
-    _Systrace : { beginEvent: () => {}, endEvent: () => {} };
-});
 
 global.__d = define;
 global.require = require;

react-packager/src/lib/ModuleTransport.js

@@ -11,6 +11,9 @@
 function ModuleTransport(data) {
   this.name = data.name;
 
+  assertExists(data, 'id');
+  this.id = data.id;
+
   assertExists(data, 'code');
   this.code = data.code;