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
2017-03-14 11:01:21 -07:00 · 2017-03-14 11:01:21 -07:00 · 4203c9c837
--- a/local-cli/bundle/output/bundle.js
+++ b/local-cli/bundle/output/bundle.js
@ -8,18 +8,21 @@
 *
 * @flow
 */
+
 'use strict';

+const Server = require('../../../packager/src/Server');
+
 const meta = require('./meta');
 const relativizeSourceMap = require('../../../packager/src//lib/relativizeSourceMap');
 const writeFile = require('./writeFile');

 import type Bundle from '../../../packager/src//Bundler/Bundle';
-import type Server from '../../../packager/src//Server';
 import type {OutputOptions, RequestOptions} from '../types.flow';

 function buildBundle(packagerClient: Server, requestOptions: RequestOptions) {
  return packagerClient.buildBundle({
+    ...Server.DEFAULT_BUNDLE_OPTIONS,
    ...requestOptions,
    isolateModuleIDs: true,
  });
--- a/local-cli/bundle/output/unbundle/index.js
+++ b/local-cli/bundle/output/unbundle/index.js
@ -8,17 +8,20 @@
 *
 * @flow
 */
+
 'use strict';

+const Server = require('../../../../packager/src/Server');
+
 const asAssets = require('./as-assets');
 const asIndexedFile = require('./as-indexed-file');

 import type Bundle from '../../../../packager/src//Bundler/Bundle';
-import type Server from '../../../../packager/src//Server';
 import type {OutputOptions, RequestOptions} from '../../types.flow';

 function buildBundle(packagerClient: Server, requestOptions: RequestOptions) {
  return packagerClient.buildBundle({
+    ...Server.DEFAULT_BUNDLE_OPTIONS,
    ...requestOptions,
    unbundle: true,
    isolateModuleIDs: true,
--- a/packager/react-packager.js
+++ b/packager/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: {}) {
--- a/packager/src/Bundler/Bundle.js
+++ b/packager/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>,
--- a/packager/src/Bundler/index.js
+++ b/packager/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(
--- a/packager/src/Server/tests/Server-test.js
+++ b/packager/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: [],
          })
        );
    });
--- a/packager/src/Server/index.js
+++ b/packager/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);
 }