packager: fix buildBundle() options

Summary: The problem with `bundleOpts` is that it discards Flow typing, so it prevents reinforcing the integration of `Bundler` into `Server`. This changeset removes the `bundleOpts` to solve that issues. Instead, it makes the options explicit so that there is less uncertaintly. I love making options explicit, because they force callsites to take a consicious decision about what is really needed, making them more robust. They also expose oddities that probably needs refatoring, for example having a `resolutionRequest` in the bundle options does not seem correct, it should be an implementation details. Likewise, `onProgress` should probably be exposed differently, as it does not affect the content of the bundle itself.

Reviewed By: davidaurelio

Differential Revision: D4697729

fbshipit-source-id: d543870ba024e7588c10b101fa51429c77cc5ddc

packages/metro-bundler/react-packager.js

@@ -42,31 +42,46 @@ type StrictOptions = {
   watch?: boolean,
 };
 
+type PublicBundleOptions = {
+  +dev?: boolean,
+  +entryFile: string,
+  +generateSourceMaps?: boolean,
+  +inlineSourceMap?: boolean,
+  +minify?: boolean,
+  +platform?: string,
+  +runModule?: boolean,
+  +sourceMapUrl?: string,
+};
+
 /**
- * This is a public API, so we don't trust the values and consider them as
- * `mixed`. Because it understands `invariant`, Flow ensure that we refine these
- * values completely.
+ * This is a public API, so we don't trust the value and purposefully downgrade
+ * it as `mixed`. Because it understands `invariant`, Flow ensure that we
+ * refine these values completely.
  */
-exports.buildBundle = function(options: Options, bundleOptions: mixed) {
-  invariant(
-    typeof bundleOptions === 'object' && bundleOptions != null,
-    '`bundleOptions` must be an object',
-  );
-  const {entryFile, platform} = bundleOptions;
-  invariant(
-    platform === undefined || typeof platform === 'string',
-    '`bundleOptions` field `platform` must be a string',
-  );
-  invariant(
-    typeof entryFile === 'string',
-    '`bundleOptions` must contain a string field `entryFile`',
-  );
+function assertPublicBundleOptions(bo: mixed): PublicBundleOptions {
+  invariant(typeof bo === 'object' && bo != null, 'bundle options must be an object');
+  invariant(bo.dev === undefined || typeof bo.dev === 'boolean', 'bundle options field `dev` must be a boolean');
+  const {entryFile} = bo;
+  invariant(typeof entryFile === 'string', 'bundle options must contain a string field `entryFile`');
+  invariant(bo.generateSourceMaps === undefined || typeof bo.generateSourceMaps === 'boolean', 'bundle options field `generateSourceMaps` must be a boolean');
+  invariant(bo.inlineSourceMap === undefined || typeof bo.inlineSourceMap === 'boolean', 'bundle options field `inlineSourceMap` must be a boolean');
+  invariant(bo.minify === undefined || typeof bo.minify === 'boolean', 'bundle options field `minify` must be a boolean');
+  invariant(bo.platform === undefined || typeof bo.platform === 'string', 'bundle options field `platform` must be a string');
+  invariant(bo.runModule === undefined || typeof bo.runModule === 'boolean', 'bundle options field `runModule` must be a boolean');
+  invariant(bo.sourceMapUrl === undefined || typeof bo.sourceMapUrl === 'string', 'bundle options field `sourceMapUrl` must be a boolean');
+  return {entryFile, ...bo};
+}
+
+exports.buildBundle = function(options: Options, bundleOptions: PublicBundleOptions) {
   var server = createNonPersistentServer(options);
-  return server.buildBundle({...bundleOptions, entryFile, platform})
-    .then(p => {
-      server.end();
-      return p;
-    });
+  const ServerClass = require('./src/Server');
+  return server.buildBundle({
+    ...ServerClass.DEFAULT_BUNDLE_OPTIONS,
+    ...assertPublicBundleOptions(bundleOptions),
+  }).then(p => {
+    server.end();
+    return p;
+  });
 };
 
 exports.getOrderedDependencyPaths = function(options: Options, bundleOptions: {}) {

packages/metro-bundler/src/Bundler/Bundle.js

@@ -44,10 +44,10 @@ class Bundle extends BundleBase {
   _ramGroups: Array<string> | void;
   _sourceMap: string | null;
   _sourceMapFormat: SourceMapFormat;
-  _sourceMapUrl: string | void;
+  _sourceMapUrl: ?string;
 
   constructor({sourceMapUrl, dev, minify, ramGroups}: {
-    sourceMapUrl?: string,
+    sourceMapUrl: ?string,
     dev?: boolean,
     minify?: boolean,
     ramGroups?: Array<string>,

packages/metro-bundler/src/Bundler/index.js

@@ -221,7 +221,7 @@ class Bundler {
     dev: boolean,
     minify: boolean,
     unbundle: boolean,
-    sourceMapUrl: string,
+    sourceMapUrl: ?string,
   }): Promise<Bundle> {
     const {dev, minify, unbundle} = options;
     return this._resolverPromise.then(

packages/metro-bundler/src/Server/__tests__/Server-test.js

@@ -6,6 +6,7 @@
  * LICENSE file in the root directory of this source tree. An additional grant
  * of patent rights can be found in the PATENTS file in the same directory.
  */
+
 'use strict';
 
 jest.disableAutomock();
@@ -144,21 +145,22 @@ describe('processRequest', () => {
     ).then(response => {
       expect(response.body).toEqual('this is the source');
       expect(Bundler.prototype.bundle).toBeCalledWith({
+        assetPlugins: [],
+        dev: true,
         entryFile: 'index.ios.js',
-        inlineSourceMap: false,
-        minify: false,
+        entryModuleOnly: false,
         generateSourceMaps: false,
         hot: false,
+        inlineSourceMap: false,
+        isolateModuleIDs: false,
+        minify: false,
+        onProgress: jasmine.any(Function),
+        platform: undefined,
+        resolutionResponse: null,
+        runBeforeMainModule: ['InitializeCore'],
         runModule: true,
         sourceMapUrl: 'index.ios.includeRequire.map',
-        dev: true,
-        platform: undefined,
-        onProgress: jasmine.any(Function),
-        runBeforeMainModule: ['InitializeCore'],
         unbundle: false,
-        entryModuleOnly: false,
-        isolateModuleIDs: false,
-        assetPlugins: [],
       });
     });
   });
@@ -170,21 +172,22 @@ describe('processRequest', () => {
     ).then(function(response) {
       expect(response.body).toEqual('this is the source');
       expect(Bundler.prototype.bundle).toBeCalledWith({
+        assetPlugins: [],
+        dev: true,
         entryFile: 'index.js',
-        inlineSourceMap: false,
-        minify: false,
+        entryModuleOnly: false,
         generateSourceMaps: false,
         hot: false,
+        inlineSourceMap: false,
+        isolateModuleIDs: false,
+        minify: false,
+        onProgress: jasmine.any(Function),
+        platform: 'ios',
+        resolutionResponse: null,
+        runBeforeMainModule: ['InitializeCore'],
         runModule: true,
         sourceMapUrl: 'index.map?platform=ios',
-        dev: true,
-        platform: 'ios',
-        onProgress: jasmine.any(Function),
-        runBeforeMainModule: ['InitializeCore'],
         unbundle: false,
-        entryModuleOnly: false,
-        isolateModuleIDs: false,
-        assetPlugins: [],
       });
     });
   });
@@ -196,21 +199,22 @@ describe('processRequest', () => {
     ).then(function(response) {
       expect(response.body).toEqual('this is the source');
       expect(Bundler.prototype.bundle).toBeCalledWith({
+        assetPlugins: ['assetPlugin1', 'assetPlugin2'],
+        dev: true,
         entryFile: 'index.js',
-        inlineSourceMap: false,
-        minify: false,
+        entryModuleOnly: false,
         generateSourceMaps: false,
         hot: false,
+        inlineSourceMap: false,
+        isolateModuleIDs: false,
+        minify: false,
+        onProgress: jasmine.any(Function),
+        platform: undefined,
+        resolutionResponse: null,
+        runBeforeMainModule: ['InitializeCore'],
         runModule: true,
         sourceMapUrl: 'index.map?assetPlugin=assetPlugin1&assetPlugin=assetPlugin2',
-        dev: true,
-        platform: undefined,
-        onProgress: jasmine.any(Function),
-        runBeforeMainModule: ['InitializeCore'],
         unbundle: false,
-        entryModuleOnly: false,
-        isolateModuleIDs: false,
-        assetPlugins: ['assetPlugin1', 'assetPlugin2'],
       });
     });
   });
@@ -422,21 +426,26 @@ describe('processRequest', () => {
   describe('buildbundle(options)', () => {
     it('Calls the bundler with the correct args', () => {
       return server.buildBundle({
+        ...Server.DEFAULT_BUNDLE_OPTIONS,
         entryFile: 'foo file',
       }).then(() =>
         expect(Bundler.prototype.bundle).toBeCalledWith({
-          entryFile: 'foo file',
-          inlineSourceMap: false,
-          minify: false,
-          hot: false,
-          runModule: true,
-          dev: true,
-          platform: undefined,
-          runBeforeMainModule: ['InitializeCore'],
-          unbundle: false,
-          entryModuleOnly: false,
-          isolateModuleIDs: false,
           assetPlugins: [],
+          dev: true,
+          entryFile: 'foo file',
+          entryModuleOnly: false,
+          generateSourceMaps: false,
+          hot: false,
+          inlineSourceMap: false,
+          isolateModuleIDs: false,
+          minify: false,
+          onProgress: null,
+          platform: undefined,
+          resolutionResponse: null,
+          runBeforeMainModule: ['InitializeCore'],
+          runModule: true,
+          sourceMapUrl: null,
+          unbundle: false,
         })
       );
     });
@@ -447,20 +456,22 @@ describe('processRequest', () => {
       return server.buildBundleFromUrl('/path/to/foo.bundle?dev=false&runModule=false')
         .then(() =>
           expect(Bundler.prototype.bundle).toBeCalledWith({
+            assetPlugins: [],
+            dev: false,
             entryFile: 'path/to/foo.js',
-            inlineSourceMap: false,
-            minify: false,
+            entryModuleOnly: false,
             generateSourceMaps: true,
             hot: false,
+            inlineSourceMap: false,
+            isolateModuleIDs: false,
+            minify: false,
+            onProgress: null,
+            platform: undefined,
+            resolutionResponse: null,
+            runBeforeMainModule: ['InitializeCore'],
             runModule: false,
             sourceMapUrl: '/path/to/foo.map?dev=false&runModule=false',
-            dev: false,
-            platform: undefined,
-            runBeforeMainModule: ['InitializeCore'],
             unbundle: false,
-            entryModuleOnly: false,
-            isolateModuleIDs: false,
-            assetPlugins: [],
           })
         );
     });

packages/metro-bundler/src/Server/index.js

@@ -77,70 +77,24 @@ type Options = {
   watch?: boolean,
 };
 
-const bundleOpts = declareOpts({
-  sourceMapUrl: {
-    type: 'string',
-    required: false,
-  },
-  entryFile: {
-    type: 'string',
-    required: true,
-  },
-  dev: {
-    type: 'boolean',
-    default: true,
-  },
-  minify: {
-    type: 'boolean',
-    default: false,
-  },
-  runModule: {
-    type: 'boolean',
-    default: true,
-  },
-  inlineSourceMap: {
-    type: 'boolean',
-    default: false,
-  },
-  platform: {
-    type: 'string',
-    required: true,
-  },
-  runBeforeMainModule: {
-    type: 'array',
-    default: defaults.runBeforeMainModule,
-  },
-  unbundle: {
-    type: 'boolean',
-    default: false,
-  },
-  hot: {
-    type: 'boolean',
-    default: false,
-  },
-  entryModuleOnly: {
-    type: 'boolean',
-    default: false,
-  },
-  isolateModuleIDs: {
-    type: 'boolean',
-    default: false,
-  },
-  resolutionResponse: {
-    type: 'object',
-  },
-  generateSourceMaps: {
-    type: 'boolean',
-    required: false,
-  },
-  assetPlugins: {
-    type: 'array',
-    default: [],
-  },
-  onProgress: {
-    type: 'function',
-  },
-});
+export type BundleOptions = {
+  +assetPlugins: Array<string>,
+  dev: boolean,
+  entryFile: string,
+  +entryModuleOnly: boolean,
+  +generateSourceMaps: boolean,
+  +hot: boolean,
+  +inlineSourceMap: boolean,
+  +isolateModuleIDs: boolean,
+  minify: boolean,
+  onProgress: ?(doneCont: number, totalCount: number) => mixed,
+  +platform: ?string,
+  +resolutionResponse: ?{},
+  +runBeforeMainModule: Array<string>,
+  +runModule: boolean,
+  sourceMapUrl: ?string,
+  unbundle: boolean,
+};
 
 const dependencyOpts = declareOpts({
   platform: {
@@ -302,12 +256,8 @@ class Server {
     }
   }
 
-  async buildBundle(options: {entryFile: string, platform?: string}): Promise<Bundle> {
-    if (!options.platform) {
-      options.platform = getPlatformExtension(options.entryFile);
-    }
-    const opts = bundleOpts(options);
-    const bundle = await this._bundler.bundle(opts);
+  async buildBundle(options: BundleOptions): Promise<Bundle> {
+    const bundle = await this._bundler.bundle(options);
     const modules = bundle.getModules();
     const nonVirtual = modules.filter(m => !m.virtual);
     bundleDeps.set(bundle, {
@@ -359,7 +309,7 @@ class Server {
 
   getDependencies(options: {
     entryFile: string,
-    platform?: string,
+    platform: ?string,
   }): Promise<ResolutionResponse> {
     return Promise.resolve().then(() => {
       if (!options.platform) {
@@ -568,10 +518,7 @@ class Server {
     });
   }
 
-  _useCachedOrUpdateOrCreateBundle(options: {
-    entryFile: string,
-    platform?: string,
-  }): Promise<Bundle> {
+  _useCachedOrUpdateOrCreateBundle(options: BundleOptions): Promise<Bundle> {
     const optionsJson = this.optionsHash(options);
     const bundleFromScratch = () => {
       const building = this.buildBundle(options);
@@ -597,8 +544,7 @@ class Server {
           // $FlowFixMe(>=0.37.0)
           deps.outdated = new Set();
 
-          const opts = bundleOpts(options);
-          const {platform, dev, minify, hot} = opts;
+          const {platform, dev, minify, hot} = options;
 
           // Need to create a resolution response to pass to the bundler
           // to process requires after transform. By providing a
@@ -880,20 +826,7 @@ class Server {
     }
   }
 
-  _getOptionsFromUrl(reqUrl: string): {
-    sourceMapUrl: string,
-    entryFile: string,
-    dev: boolean,
-    minify: boolean,
-    hot: boolean,
-    runModule: boolean,
-    inlineSourceMap: boolean,
-    platform?: string,
-    entryModuleOnly: boolean,
-    generateSourceMaps: boolean,
-    assetPlugins: Array<string>,
-    onProgress?: (doneCont: number, totalCount: number) => mixed,
-  } {
+  _getOptionsFromUrl(reqUrl: string): BundleOptions {
     // `true` to parse the query param as an object.
     const urlObj = url.parse(reqUrl, true);
 
@@ -934,13 +867,16 @@ class Server {
       dev,
       minify,
       hot: this._getBoolOptionFromQuery(urlObj.query, 'hot', false),
+      runBeforeMainModule: defaults.runBeforeMainModule,
       runModule: this._getBoolOptionFromQuery(urlObj.query, 'runModule', true),
       inlineSourceMap: this._getBoolOptionFromQuery(
         urlObj.query,
         'inlineSourceMap',
         false
       ),
+      isolateModuleIDs: false,
       platform,
+      resolutionResponse: null,
       entryModuleOnly: this._getBoolOptionFromQuery(
         urlObj.query,
         'entryModuleOnly',
@@ -949,6 +885,8 @@ class Server {
       generateSourceMaps:
         minify || !dev || this._getBoolOptionFromQuery(urlObj.query, 'babelSourcemap', false),
       assetPlugins,
+      onProgress: null,
+      unbundle: false,
     };
   }
 
@@ -960,8 +898,28 @@ class Server {
 
     return query[opt] === 'true' || query[opt] === '1';
   }
+
+  static DEFAULT_BUNDLE_OPTIONS;
+
 }
 
+Server.DEFAULT_BUNDLE_OPTIONS =  {
+  assetPlugins: [],
+  dev: true,
+  entryModuleOnly: false,
+  generateSourceMaps: false,
+  hot: false,
+  inlineSourceMap: false,
+  isolateModuleIDs: false,
+  minify: false,
+  onProgress: null,
+  resolutionResponse: null,
+  runBeforeMainModule: defaults.runBeforeMainModule,
+  runModule: true,
+  sourceMapUrl: null,
+  unbundle: false,
+};
+
 function contentsEqual<T>(array: Array<T>, set: Set<T>): boolean {
   return array.length === set.size && array.every(set.has, set);
 }