`GraphFn` returns promise rather than taking callback

Summary: Changes `GraphFn` to return a promise rather than taking a callback. This is more in line with call sites, which so far have been using `denodeify`.

Reviewed By: jeanlauliac

Differential Revision: D5857192

fbshipit-source-id: 61bbbef4fbf24b0b91c4a3008541eaa5a1af0f7a

packages/metro-bundler/src/ModuleGraph/Graph.js

@@ -57,7 +57,7 @@ exports.create = function create(resolve: ResolveFn, load: LoadFn): GraphFn {
   const resolveCallback = asyncify(resolve);
   const loadCallback = asyncify(load);
 
-  function Graph(entryPoints, platform, options, callback = emptyFunction) {
+  function Graph(entryPoints, platform, options) {
     const {
       log = (console: any),
       optimize = false,
@@ -66,8 +66,7 @@ exports.create = function create(resolve: ResolveFn, load: LoadFn): GraphFn {
 
     if (typeof platform !== 'string') {
       log.error('`Graph`, called without a platform');
-      callback(Error('The target platform has to be passed'));
+      return Promise.reject(new Error('The target platform has to be passed'));
-      return;
     }
 
     const loadQueue: LoadQueue = queue(seq(
@@ -78,14 +77,16 @@ exports.create = function create(resolve: ResolveFn, load: LoadFn): GraphFn {
 
     const {collect, loadModule} = createGraphHelpers(loadQueue, skip);
 
+    const result = deferred();
+
     loadQueue.drain = () => {
       loadQueue.kill();
-      callback(null, collect());
+      result.resolve(collect());
     };
     loadQueue.error = error => {
       loadQueue.error = emptyFunction;
       loadQueue.kill();
-      callback(error);
+      result.reject(error);
     };
 
     let i = 0;
@@ -96,8 +97,10 @@ exports.create = function create(resolve: ResolveFn, load: LoadFn): GraphFn {
     if (i === 0) {
       log.error('`Graph` called without any entry points');
       loadQueue.kill();
-      callback(Error('At least one entry point has to be passed.'));
+      return Promise.reject(new Error('At least one entry point has to be passed.'));
     }
+
+    return result.promise;
   }
 
   return Graph;
@@ -173,3 +176,17 @@ function createGraphHelpers(loadQueue, skip) {
 
   return {collect, loadModule};
 }
+
+function deferred<T>(): {
+  promise: Promise<T>,
+  reject: Error => void,
+  resolve: T => void,
+} {
+  let reject, resolve;
+  const promise = new Promise((res, rej) => {
+    reject = rej;
+    resolve = res;
+  });
+
+  return {promise, reject: nullthrows(reject), resolve: nullthrows(resolve)};
+}

packages/metro-bundler/src/ModuleGraph/ModuleGraph.js

@@ -12,15 +12,9 @@
 'use strict';
 
 const defaults = require('../defaults');
-const denodeify: Denodeify = require('denodeify');
 const virtualModule = require('./module').virtual;
 
-import type {
+import type {BuildResult, GraphFn, PostProcessModules} from './types.flow';
-  BuildResult,
-  Callback,
-  GraphFn,
-  PostProcessModules,
-} from './types.flow';
 
 export type BuildFn = (
   entryPoints: Iterable<string>,
@@ -33,10 +27,6 @@ type BuildOptions = {|
   platform: string,
 |};
 
-type Denodeify = <A, B, C, T>(
-  (A, B, C, Callback<T>) => void,
-) => (A, B, C) => Promise<T>;
-
 exports.createBuildSetup = (
   graphFn: GraphFn,
   postProcessModules: PostProcessModules,
@@ -49,8 +39,7 @@ exports.createBuildSetup = (
   } = options;
   const graphOptions = {optimize};
 
-  const pgraph = denodeify(graphFn);
+  const graphWithOptions = entry => graphFn(entry, platform, graphOptions);
-  const graphWithOptions = entry => pgraph(entry, platform, graphOptions);
   const graphOnlyModules = async m => (await graphWithOptions(m)).modules;
 
   const [graph, moduleSystem, polyfills] = await Promise.all([

packages/metro-bundler/src/ModuleGraph/__tests__/Graph-test.js

@@ -35,29 +35,26 @@ describe('Graph:', () => {
     graph = Graph.create(resolve, load);
   });
 
-  it('calls back an error when called without any entry point', done => {
+  it('calls back an error when called without any entry point', async () => {
-    graph([], anyPlatform, {log: quiet}, error => {
+    expect.assertions(1);
+    try {
+      await graph([], anyPlatform, {log: quiet});
+    } catch (error) {
       expect(error).toEqual(any(Error));
-      done();
+    }
-    });
   });
 
-  it('resolves the entry point with the passed-in `resolve` function', done => {
+  it('resolves the entry point with the passed-in `resolve` function', async () => {
     const entryPoint = '/arbitrary/path';
-    graph([entryPoint], anyPlatform, noOpts, () => {
+    await graph([entryPoint], anyPlatform, noOpts);
-      expect(resolve).toBeCalledWith(
+
-        entryPoint,
+    expect(resolve).toBeCalledWith(entryPoint, null, any(String), any(Object));
-        null,
-        any(String),
-        any(Object),
-      );
-      done();
-    });
   });
 
-  it('allows to specify multiple entry points', done => {
+  it('allows to specify multiple entry points', async () => {
     const entryPoints = ['Arbitrary', '../entry.js'];
-    graph(entryPoints, anyPlatform, noOpts, () => {
+    await graph(entryPoints, anyPlatform, noOpts);
+
     expect(resolve).toBeCalledWith(
       entryPoints[0],
       null,
@@ -70,73 +67,48 @@ describe('Graph:', () => {
       any(String),
       any(Object),
     );
-      done();
-    });
   });
 
-  it('calls back with an error when called without `platform` option', done => {
+  it('calls back with an error when called without `platform` option', async () => {
-    graph(anyEntry, undefined, {log: quiet}, error => {
+    expect.assertions(1);
+    try {
+      await graph(anyEntry, undefined, {log: quiet});
+    } catch (error) {
       expect(error).toEqual(any(Error));
-      done();
+    }
-    });
   });
 
-  it('forwards a passed-in `platform` to `resolve`', done => {
+  it('forwards a passed-in `platform` to `resolve`', async () => {
     const platform = 'any';
-    graph(anyEntry, platform, noOpts, () => {
+    await graph(anyEntry, platform, noOpts);
+
     expect(resolve).toBeCalledWith(any(String), null, platform, any(Object));
-      done();
-    });
   });
 
-  it('forwards a passed-in `log` option to `resolve`', done => {
+  it('forwards a passed-in `log` option to `resolve`', async () => {
     const log = new Console();
-    graph(anyEntry, anyPlatform, {log}, () => {
+    await graph(anyEntry, anyPlatform, {log});
     expect(resolve).toBeCalledWith(
       any(String),
       null,
       any(String),
       objectContaining({log}),
     );
-      done();
-    });
   });
 
-  it('calls back with every error produced by `resolve`', done => {
+  it('calls back with every error produced by `resolve`', async () => {
+    expect.assertions(1);
     const error = Error();
     resolve.stub.throws(error);
-    graph(anyEntry, anyPlatform, noOpts, e => {
+
+    try {
+      await graph(anyEntry, anyPlatform, noOpts);
+    } catch (e) {
       expect(e).toBe(error);
-      done();
-    });
-  });
-
-  it('only calls back once if two parallel invocations of `resolve` fail', done => {
-    load.stub.returns({
-      file: createFile('with two deps'),
-      dependencies: ['depA', 'depB'],
-    });
-    resolve.stub
-      .withArgs('depA')
-      .throws(new Error())
-      .withArgs('depB')
-      .throws(new Error());
-
-    let calls = 0;
-    function callback() {
-      if (calls === 0) {
-        process.nextTick(() => {
-          expect(calls).toEqual(1);
-          done();
-        });
     }
-      ++calls;
-    }
-
-    graph(['entryA', 'entryB'], anyPlatform, noOpts, callback);
   });
 
-  it('passes the files returned by `resolve` on to the `load` function', done => {
+  it('passes the files returned by `resolve` on to the `load` function', async () => {
     const modules = new Map([
       ['Arbitrary', '/absolute/path/to/Arbitrary.js'],
       ['../entry.js', '/whereever/is/entry.js'],
@@ -146,51 +118,46 @@ describe('Graph:', () => {
     }
     const [file1, file2] = modules.values();
 
-    graph(modules.keys(), anyPlatform, noOpts, () => {
+    await graph(modules.keys(), anyPlatform, noOpts);
     expect(load).toBeCalledWith(file1, any(Object));
     expect(load).toBeCalledWith(file2, any(Object));
-      done();
-    });
   });
 
-  it('passes the `optimize` flag on to `load`', done => {
+  it('passes the `optimize` flag on to `load`', async () => {
-    graph(anyEntry, anyPlatform, {optimize: true}, () => {
+    await graph(anyEntry, anyPlatform, {optimize: true});
     expect(load).toBeCalledWith(
       any(String),
       objectContaining({optimize: true}),
     );
-      done();
-    });
   });
 
-  it('uses `false` as the default for the `optimize` flag', done => {
+  it('uses `false` as the default for the `optimize` flag', async () => {
-    graph(anyEntry, anyPlatform, noOpts, () => {
+    await graph(anyEntry, anyPlatform, noOpts);
     expect(load).toBeCalledWith(
       any(String),
       objectContaining({optimize: false}),
     );
-      done();
-    });
   });
 
-  it('forwards a passed-in `log` to `load`', done => {
+  it('forwards a passed-in `log` to `load`', async () => {
     const log = new Console();
-    graph(anyEntry, anyPlatform, {log}, () => {
+    await graph(anyEntry, anyPlatform, {log});
     expect(load).toBeCalledWith(any(String), objectContaining({log}));
-      done();
-    });
   });
 
-  it('calls back with every error produced by `load`', done => {
+  it('calls back with every error produced by `load`', async () => {
+    expect.assertions(1);
     const error = Error();
     load.stub.throws(error);
-    graph(anyEntry, anyPlatform, noOpts, e => {
+
+    try {
+      await graph(anyEntry, anyPlatform, noOpts);
+    } catch (e) {
       expect(e).toBe(error);
-      done();
+    }
-    });
   });
 
-  it('resolves any dependencies provided by `load`', done => {
+  it('resolves any dependencies provided by `load`', async () => {
     const entryPath = '/path/to/entry.js';
     const id1 = 'required/id';
     const id2 = './relative/import';
@@ -200,14 +167,12 @@ describe('Graph:', () => {
       dependencies: [id1, id2],
     });
 
-    graph(['entry'], anyPlatform, noOpts, () => {
+    await graph(['entry'], anyPlatform, noOpts);
     expect(resolve).toBeCalledWith(id1, entryPath, any(String), any(Object));
     expect(resolve).toBeCalledWith(id2, entryPath, any(String), any(Object));
-      done();
-    });
   });
 
-  it('loads transitive dependencies', done => {
+  it('loads transitive dependencies', async () => {
     const entryPath = '/path/to/entry.js';
     const id1 = 'required/id';
     const id2 = './relative/import';
@@ -227,15 +192,13 @@ describe('Graph:', () => {
       .withArgs(path1)
       .returns({file: {path: path1}, dependencies: [id2]});
 
-    graph(['entry'], anyPlatform, noOpts, () => {
+    await graph(['entry'], anyPlatform, noOpts);
     expect(resolve).toBeCalledWith(id2, path1, any(String), any(Object));
     expect(load).toBeCalledWith(path1, any(Object));
     expect(load).toBeCalledWith(path2, any(Object));
-      done();
-    });
   });
 
-  it('resolves modules in depth-first traversal order, regardless of the order of loading', done => {
+  it('resolves modules in depth-first traversal order, regardless of the order of loading', async () => {
     load.stub.reset();
     resolve.stub.reset();
 
@@ -269,8 +232,7 @@ describe('Graph:', () => {
       return {file: createFile('h'), dependencies: []};
     };
 
-    graph(['a'], anyPlatform, noOpts, (error, result) => {
+    const result = await graph(['a'], anyPlatform, noOpts);
-      expect(error).toEqual(null);
     expect(result.modules).toEqual([
       createModule('a', ['b', 'e', 'h']),
       createModule('b', ['c', 'd']),
@@ -281,11 +243,9 @@ describe('Graph:', () => {
       createModule('g'),
       createModule('h'),
     ]);
-      done();
-    });
   });
 
-  it('calls back with the resolved modules of the entry points', done => {
+  it('calls back with the resolved modules of the entry points', async () => {
     load.stub.reset();
     resolve.stub.reset();
 
@@ -306,16 +266,14 @@ describe('Graph:', () => {
       .split('')
       .forEach(id => resolve.stub.withArgs(id).returns(idToPath(id)));
 
-    graph(['a', 'c'], anyPlatform, noOpts, (error, result) => {
+    const result = await graph(['a', 'c'], anyPlatform, noOpts);
     expect(result.entryModules).toEqual([
       createModule('a', ['b']),
       createModule('c', ['d']),
     ]);
-      done();
-    });
   });
 
-  it('resolves modules for all entry points correctly if one is a dependency of another', done => {
+  it('resolves modules for all entry points correctly if one is a dependency of another', async () => {
     load.stub.reset();
     resolve.stub.reset();
 
@@ -330,16 +288,14 @@ describe('Graph:', () => {
       .split('')
       .forEach(id => resolve.stub.withArgs(id).returns(idToPath(id)));
 
-    graph(['a', 'b'], anyPlatform, noOpts, (error, result) => {
+    const result = await graph(['a', 'b'], anyPlatform, noOpts);
     expect(result.entryModules).toEqual([
       createModule('a', ['b']),
       createModule('b', []),
     ]);
-      done();
-    });
   });
 
-  it('does not include dependencies more than once', done => {
+  it('does not include dependencies more than once', async () => {
     const ids = ['a', 'b', 'c', 'd'];
     ids.forEach(id => {
       const path = idToPath(id);
@@ -354,19 +310,16 @@ describe('Graph:', () => {
         .returns({file: createFile(id), dependencies: ['b', 'c']}),
     );
 
-    graph(['a', 'd', 'b'], anyPlatform, noOpts, (error, result) => {
+    const result = await graph(['a', 'd', 'b'], anyPlatform, noOpts);
-      expect(error).toEqual(null);
     expect(result.modules).toEqual([
       createModule('a', ['b', 'c']),
       createModule('b'),
       createModule('c'),
       createModule('d', ['b', 'c']),
     ]);
-      done();
-    });
   });
 
-  it('handles dependency cycles', done => {
+  it('handles dependency cycles', async () => {
     resolve.stub
       .withArgs('a')
       .returns(idToPath('a'))
@@ -382,17 +335,15 @@ describe('Graph:', () => {
       .withArgs(idToPath('c'))
       .returns({file: createFile('c'), dependencies: ['a']});
 
-    graph(['a'], anyPlatform, noOpts, (error, result) => {
+    const result = await graph(['a'], anyPlatform, noOpts);
     expect(result.modules).toEqual([
       createModule('a', ['b']),
       createModule('b', ['c']),
       createModule('c', ['a']),
     ]);
-      done();
-    });
   });
 
-  it('can skip files', done => {
+  it('can skip files', async () => {
     ['a', 'b', 'c', 'd', 'e'].forEach(id =>
       resolve.stub.withArgs(id).returns(idToPath(id)),
     );
@@ -408,13 +359,11 @@ describe('Graph:', () => {
     );
     const skip = new Set([idToPath('b'), idToPath('c')]);
 
-    graph(['a'], anyPlatform, {skip}, (error, result) => {
+    const result = await graph(['a'], anyPlatform, {skip});
     expect(result.modules).toEqual([
       createModule('a', ['b', 'c', 'd']),
       createModule('d', []),
     ]);
-      done();
-    });
   });
 });
 

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

@@ -92,14 +92,14 @@ function moduleFromPath(path) {
   };
 }
 
-function graph(entryPoints, platform, options, callback) {
+async function graph(entryPoints, platform, options, callback) {
   const modules = Array.from(entryPoints, moduleFromPath);
   const depModules = Array.prototype.concat.apply(
     [],
     modules.map(x => x.dependencies.map(moduleFromPath)),
   );
-  callback(null, {
+  return {
     entryModules: modules,
     modules: modules.concat(depModules),
-  });
+  };
 }

packages/metro-bundler/src/ModuleGraph/types.flow.js

@@ -42,8 +42,7 @@ export type GraphFn = (
   entryPoints: Iterable<string>,
   platform: string,
   options?: ?GraphOptions,
-  callback?: Callback<GraphResult>,
+) => Promise<GraphResult>;
-) => void;
 
 type GraphOptions = {|
   log?: Console,