Clean up Bundler

Summary:
The bundler class had duplicated code and parts that were hard to follow, because functions accepted heterogenous arguments, leading to a lot of conditionals.

This commit tries to remove duplication between build steps of different type of bundles, and by accepting less different types of arguments. These differences are now handled further up the call stack.

public

Reviewed By: sebmarkbage

Differential Revision: D2905807

fb-gh-sync-id: ef85ea0d461a9a06a4a64480e014a5324c4ef532

local-cli/server/util/attachHMRServer.js

@@ -137,10 +137,7 @@ function attachHMRServer({httpServer, path, packagerServer}) {
                   }).then(response => {
                     const module = packagerServer.getModuleForPath(filename);
 
-                    return {
-                      modulesToUpdate: [module],
-                      resolutionResponse: response,
-                    };
+                    return response.copy({dependencies: [module]});
                   });
                 }
 
@@ -170,13 +167,12 @@ function attachHMRServer({httpServer, path, packagerServer}) {
                     client.dependenciesModulesCache = dependenciesModulesCache;
                     client.shallowDependencies = shallowDependencies;
 
-                    return {
-                      modulesToUpdate,
-                      resolutionResponse,
-                    };
+                    return resolutionResponse.copy({
+                      dependencies: modulesToUpdate
+                    });
                   });
               })
-              .then(({modulesToUpdate, resolutionResponse}) => {
+              .then((resolutionResponse) => {
                 if (!client) {
                   return;
                 }
@@ -189,9 +185,8 @@ function attachHMRServer({httpServer, path, packagerServer}) {
                 return packagerServer.buildBundleForHMR({
                   entryFile: client.bundleEntry,
                   platform: client.platform,
-                  modules: modulesToUpdate,
                   resolutionResponse,
-                })
+                });
               })
               .then(bundle => {
                 if (!client || !bundle || bundle.isEmpty()) {

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

@@ -16,7 +16,7 @@ class BundleBase {
     this._finalized = false;
     this._modules = [];
     this._assets = [];
-    this._mainModuleId = this._mainModuleName = undefined;
+    this._mainModuleId = undefined;
   }
 
   isEmpty() {
@@ -31,14 +31,6 @@ class BundleBase {
     this._mainModuleId = moduleId;
   }
 
-  getMainModuleName() {
-    return this._mainModuleName;
-  }
-
-  setMainModuleName(moduleName) {
-    this._mainModuleName = moduleName;
-  }
-
   addModule(module) {
     if (!module instanceof ModuleTransport) {
       throw new Error('Expeceted a ModuleTransport object');
@@ -89,7 +81,6 @@ class BundleBase {
       modules: this._modules,
       assets: this._assets,
       mainModuleId: this.getMainModuleId(),
-      mainModuleName: this.getMainModuleName(),
     };
   }
 
@@ -97,7 +88,6 @@ class BundleBase {
     bundle._assets = json.assets;
     bundle._modules = json.modules;
     bundle.setMainModuleId(json.mainModuleId);
-    bundle.setMainModuleName(json.mainModuleName);
 
     Object.freeze(bundle._modules);
     Object.seal(bundle._modules);

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

@@ -312,28 +312,21 @@ describe('Bundle', () => {
     });
   });
 
-  describe('main module name and id:', function() {
-    it('keeps distinct module names and IDs', function() {
+  describe('main module id:', function() {
+    it('can save a main module ID', function() {
       const id = 'arbitrary module ID';
-      const name = 'arbitrary module name';
       bundle.setMainModuleId(id);
-      bundle.setMainModuleName(name);
-
       expect(bundle.getMainModuleId()).toEqual(id);
-      expect(bundle.getMainModuleName()).toEqual(name);
     });
 
-    it('can serialize and deserialize module ID and name', function() {
+    it('can serialize and deserialize the module ID', function() {
       const id = 'arbitrary module ID';
-      const name = 'arbitrary module name';
       bundle.setMainModuleId(id);
-      bundle.setMainModuleName(name);
       bundle.finalize({});
 
       const deserialized = Bundle.fromJSON(bundle.toJSON());
 
       expect(deserialized.getMainModuleId()).toEqual(id);
-      expect(deserialized.getMainModuleName()).toEqual(name);
     });
   });
 });

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

@@ -29,6 +29,8 @@ const version = require('../../../../package.json').version;
 const sizeOf = Promise.denodeify(imageSize);
 const readFile = Promise.denodeify(fs.readFile);
 
+const noop = () => {};
+
 const validateOpts = declareOpts({
   projectRoots: {
     type: 'array',
@@ -159,9 +161,12 @@ class Bundler {
   }
 
   bundle(options) {
+    const {dev, isUnbundle, platform} = options;
+    const moduleSystemDeps =
+      this._resolver.getModuleSystemDependencies({dev, isUnbundle, platform});
     return this._bundle({
       bundle: new Bundle(options.sourceMapUrl),
-      includeSystemDependencies: true,
+      moduleSystemDeps,
       ...options,
     });
   }
@@ -209,7 +214,7 @@ class Bundler {
     );
   }
 
-  bundleForHMR(options) {
+  hmrBundle(options) {
     return this._bundle({
       bundle: new HMRBundle({
         sourceURLFn: this._sourceHMRURL.bind(this, options.platform),
@@ -225,122 +230,51 @@ class Bundler {
 
   _bundle({
     bundle,
-    modules,
     entryFile,
     runModule: runMainModule,
     runBeforeMainModule,
     dev: isDev,
-    includeSystemDependencies,
     platform,
-    unbundle: isUnbundle,
-    hot: hot,
+    moduleSystemDeps = [],
+    hot,
     entryModuleOnly,
-    resolutionResponse,
+    resolutionResponse
   }) {
-    let transformEventId;
-    const moduleSystemDeps = includeSystemDependencies
-      ? this._resolver.getModuleSystemDependencies(
-        { dev: isDev, platform, isUnbundle }
-      )
-      : [];
-
-    const findModules = () => {
-      const findEventId = Activity.startEvent('find dependencies');
-      return this.getDependencies(entryFile, isDev, platform).then(response => {
-        Activity.endEvent(findEventId);
+    const onResolutionResponse = response => {
       bundle.setMainModuleId(response.mainModuleId);
-        bundle.setMainModuleName(response.mainModuleId);
-        if (!entryModuleOnly && bundle.setNumPrependedModules) {
+      if (bundle.setNumPrependedModules) {
         bundle.setNumPrependedModules(
           response.numPrependedDependencies + moduleSystemDeps.length
         );
       }
-
-        return {
-          response,
-          modulesToProcess: response.dependencies,
-        };
-      });
-    };
-
-    const useProvidedModules = () => {
-      const moduleId = this._resolver.getModuleForPath(entryFile);
-      bundle.setMainModuleId(moduleId);
-      bundle.setMainModuleName(moduleId);
-      return Promise.resolve({
-        response: resolutionResponse,
-        modulesToProcess: modules
-      });
-    };
-
-    return (
-      modules ? useProvidedModules() : findModules()
-    ).then(({response, modulesToProcess}) => {
-
-      transformEventId = Activity.startEvent('transform');
-
-      let dependencies;
       if (entryModuleOnly) {
-        dependencies = response.dependencies.filter(module =>
+        response.dependencies = response.dependencies.filter(module =>
           module.path.endsWith(entryFile)
         );
       } else {
-        const moduleSystemDeps = includeSystemDependencies
-          ? this._resolver.getModuleSystemDependencies(
-            { dev: isDev, platform, isUnbundle }
-          )
-          : [];
-
-        const modulesToProcess = modules || response.dependencies;
-        dependencies = moduleSystemDeps.concat(modulesToProcess);
+        response.dependencies = moduleSystemDeps.concat(response.dependencies);
       }
-
-      let bar;
-      if (process.stdout.isTTY) {
-        bar = new ProgressBar('transforming [:bar] :percent :current/:total', {
-          complete: '=',
-          incomplete: ' ',
-          width: 40,
-          total: dependencies.length,
-        });
-      }
-
-      return Promise.all(
-        dependencies.map(
-          module => {
-            return this._transformModule(
-              bundle,
-              module,
-              platform,
-              isDev,
-              hot,
-            ).then(transformed => {
-              if (bar) {
-                bar.tick();
-              }
-
-              return {
-                module,
-                transformed,
     };
-            });
-          }
+    const finalizeBundle = ({bundle, transformedModules, response}) =>
+      Promise.all(
+        transformedModules.map(({module, transformed}) =>
+          bundle.addModule(this._resolver, response, module, transformed)
         )
-      ).then(transformedModules => Promise.all(
-        transformedModules.map(({module, transformed}) => {
-          return bundle.addModule(
-            this._resolver,
-            response,
-            module,
-            transformed,
-          );
-        })
-      ));
-    }).then(() => {
-      Activity.endEvent(transformEventId);
+      ).then(() => {
         bundle.finalize({runBeforeMainModule, runMainModule});
         return bundle;
       });
+
+    return this._buildBundle({
+      entryFile,
+      isDev,
+      platform,
+      bundle,
+      hot,
+      resolutionResponse,
+      onResolutionResponse,
+      finalizeBundle,
+    });
   }
 
   prepackBundle({
@@ -351,41 +285,9 @@ class Bundler {
     dev: isDev,
     platform,
   }) {
-    const bundle = new PrepackBundle(sourceMapUrl);
-    const findEventId = Activity.startEvent('find dependencies');
-    let transformEventId;
-    let mainModuleId;
-
-    return this.getDependencies(entryFile, isDev, platform).then((response) => {
-      Activity.endEvent(findEventId);
-      transformEventId = Activity.startEvent('transform');
-
-      let bar;
-      if (process.stdout.isTTY) {
-        bar = new ProgressBar('transforming [:bar] :percent :current/:total', {
-          complete: '=',
-          incomplete: ' ',
-          width: 40,
-          total: response.dependencies.length,
-        });
-      }
-
-      mainModuleId = response.mainModuleId;
-
-      return Promise.all(
-        response.dependencies.map(
-          module => this._transformModule(
-            bundle,
-            module,
-            platform,
-            isDev,
-          ).then(transformed => {
-            if (bar) {
-              bar.tick();
-            }
-
-            var deps = Object.create(null);
-            var pairs = response.getResolvedDependencyPairs(module);
+    const onModuleTransformed = ({module, transformed, response, bundle}) => {
+      const deps = Object.create(null);
+      const pairs = response.getResolvedDependencyPairs(module);
       if (pairs) {
         pairs.forEach(pair => {
           deps[pair[0]] = pair[1].path;
@@ -395,34 +297,74 @@ class Bundler {
       return module.getName().then(name => {
         bundle.addModule(name, transformed, deps, module.isPolyfill());
       });
-          })
-        )
-      );
-    }).then(() => {
-      Activity.endEvent(transformEventId);
-      bundle.finalize({runBeforeMainModule, runMainModule, mainModuleId });
+    };
+    const finalizeBundle = ({bundle, response}) => {
+      const {mainModuleId} = response;
+      bundle.finalize({runBeforeMainModule, runMainModule, mainModuleId});
       return bundle;
+    };
+
+    return this._buildBundle({
+      entryFile,
+      isDev,
+      platform,
+      onModuleTransformed,
+      finalizeBundle,
+      bundle: new PrepackBundle(sourceMapUrl),
     });
   }
 
-  _transformModuleForHMR(module, platform) {
-    if (module.isAsset()) {
-      return this._generateAssetObjAndCode(module, platform).then(
-        ({asset, code}) => {
-          return {
-            code,
-          };
+  _buildBundle({
+    entryFile,
+    isDev,
+    platform,
+    bundle,
+    hot,
+    resolutionResponse,
+    onResolutionResponse = noop,
+    onModuleTransformed = noop,
+    finalizeBundle = noop,
+  }) {
+    const findEventId = Activity.startEvent('find dependencies');
+    if (!resolutionResponse) {
+      resolutionResponse = this.getDependencies(entryFile, isDev, platform);
     }
+
+    return Promise.resolve(resolutionResponse).then(response => {
+      Activity.endEvent(findEventId);
+      onResolutionResponse(response);
+
+      const transformEventId = Activity.startEvent('transform');
+      const bar = process.stdout.isTTY
+          ? new ProgressBar('transforming [:bar] :percent :current/:total', {
+              complete: '=',
+              incomplete: ' ',
+              width: 40,
+              total: response.dependencies.length,
+            })
+          : {tick() {}};
+      const transformPromises =
+        response.dependencies.map(module =>
+          this._transformModule({
+            mainModuleName: response.mainModuleId,
+            bundle,
+            module,
+            platform,
+            dev: isDev,
+            hot
+          }).then(transformed => {
+            bar.tick();
+            onModuleTransformed({module, transformed, response, bundle});
+            return {module, transformed};
+          })
         );
-    } else {
-      // TODO(martinb): pass non null main (t9527509)
-      return this._getTransformOptions(
-        {main: null, dev: true, platform: 'ios'}, // TODO(martinb): avoid hard-coding platform
-        {hot: true},
-      ).then(options => {
-        return this._transformer.loadFileAndTransform(module.path, options);
+      return Promise.all(transformPromises).then(transformedModules => {
+        Activity.endEvent(transformEventId);
+        return Promise
+          .resolve(finalizeBundle({bundle, transformedModules, response}))
+          .then(() => bundle);
+      });
     });
-    }
   }
 
   invalidateFile(filePath) {
@@ -489,7 +431,14 @@ class Bundler {
     );
   }
 
-  _transformModule(bundle, module, platform = null, dev = true, hot = false) {
+  _transformModule({
+    bundle,
+    module,
+    mainModuleName,
+    platform = null,
+    dev = true,
+    hot = false,
+  }) {
     if (module.isAsset_DEPRECATED()) {
       return this._generateAssetModule_DEPRECATED(bundle, module);
     } else if (module.isAsset()) {
@@ -499,12 +448,12 @@ class Bundler {
     } else {
       return this._getTransformOptions(
         {
-          bundleEntry: bundle.getMainModuleName(),
+          bundleEntry: mainModuleName,
           platform: platform,
           dev: dev,
           modulePath: module.path,
         },
-        {hot: hot},
+        {hot},
       ).then(options => {
         return this._transformer.loadFileAndTransform(
           path.resolve(module.path),

packager/react-packager/src/DependencyResolver/DependencyGraph/ResolutionResponse.js

@@ -19,6 +19,21 @@ class ResolutionResponse {
     this._finalized = false;
   }
 
+  copy(properties) {
+    const {
+      dependencies = this.dependencies,
+      asyncDependencies = this.asyncDependencies,
+      mainModuleId = this.mainModuleId,
+      mocks = this.mocks,
+    } = properties;
+    return Object.assign(new this.constructor(), this, {
+      dependencies,
+      asyncDependencies,
+      mainModuleId,
+      mocks,
+    });
+  }
+
   _assertNotFinalized() {
     if (this._finalized) {
       throw new Error('Attempted to mutate finalized response.');

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

@@ -241,7 +241,7 @@ class Server {
   }
 
   buildBundleForHMR(modules) {
-    return this._bundler.bundleForHMR(modules);
+    return this._bundler.hmrBundle(modules);
   }
 
   getShallowDependencies(entryFile) {