From c546349da467d85b69f15a9285e3aa0ca5180bbe Mon Sep 17 00:00:00 2001 From: David Aurelio Date: Tue, 19 Sep 2017 09:18:53 -0700 Subject: [PATCH] `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 --- .../metro-bundler/src/ModuleGraph/Graph.js | 29 +- .../src/ModuleGraph/ModuleGraph.js | 15 +- .../src/ModuleGraph/__tests__/Graph-test.js | 309 ++++++++---------- .../ModuleGraph/__tests__/ModuleGraph-test.js | 6 +- .../src/ModuleGraph/types.flow.js | 3 +- 5 files changed, 158 insertions(+), 204 deletions(-) diff --git a/packages/metro-bundler/src/ModuleGraph/Graph.js b/packages/metro-bundler/src/ModuleGraph/Graph.js index 66f09620..82f82bf8 100644 --- a/packages/metro-bundler/src/ModuleGraph/Graph.js +++ b/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; + return Promise.reject(new Error('The target platform has to be passed')); } 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(): { + promise: Promise, + 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)}; +} diff --git a/packages/metro-bundler/src/ModuleGraph/ModuleGraph.js b/packages/metro-bundler/src/ModuleGraph/ModuleGraph.js index a56ae701..e6491049 100644 --- a/packages/metro-bundler/src/ModuleGraph/ModuleGraph.js +++ b/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 { - BuildResult, - Callback, - GraphFn, - PostProcessModules, -} from './types.flow'; +import type {BuildResult, GraphFn, PostProcessModules} from './types.flow'; export type BuildFn = ( entryPoints: Iterable, @@ -33,10 +27,6 @@ type BuildOptions = {| platform: string, |}; -type Denodeify = ( - (A, B, C, Callback) => void, -) => (A, B, C) => Promise; - exports.createBuildSetup = ( graphFn: GraphFn, postProcessModules: PostProcessModules, @@ -49,8 +39,7 @@ exports.createBuildSetup = ( } = options; const graphOptions = {optimize}; - const pgraph = denodeify(graphFn); - const graphWithOptions = entry => pgraph(entry, platform, graphOptions); + const graphWithOptions = entry => graphFn(entry, platform, graphOptions); const graphOnlyModules = async m => (await graphWithOptions(m)).modules; const [graph, moduleSystem, polyfills] = await Promise.all([ diff --git a/packages/metro-bundler/src/ModuleGraph/__tests__/Graph-test.js b/packages/metro-bundler/src/ModuleGraph/__tests__/Graph-test.js index 288e5550..e2415fff 100644 --- a/packages/metro-bundler/src/ModuleGraph/__tests__/Graph-test.js +++ b/packages/metro-bundler/src/ModuleGraph/__tests__/Graph-test.js @@ -35,108 +35,80 @@ describe('Graph:', () => { graph = Graph.create(resolve, load); }); - it('calls back an error when called without any entry point', done => { - graph([], anyPlatform, {log: quiet}, error => { + it('calls back an error when called without any entry point', async () => { + 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, () => { - expect(resolve).toBeCalledWith( - entryPoint, - null, - any(String), - any(Object), - ); - done(); - }); + await graph([entryPoint], anyPlatform, noOpts); + + expect(resolve).toBeCalledWith(entryPoint, null, any(String), any(Object)); }); - 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, () => { - expect(resolve).toBeCalledWith( - entryPoints[0], - null, - any(String), - any(Object), - ); - expect(resolve).toBeCalledWith( - entryPoints[1], - null, - any(String), - any(Object), - ); - done(); - }); + await graph(entryPoints, anyPlatform, noOpts); + + expect(resolve).toBeCalledWith( + entryPoints[0], + null, + any(String), + any(Object), + ); + expect(resolve).toBeCalledWith( + entryPoints[1], + null, + any(String), + any(Object), + ); }); - it('calls back with an error when called without `platform` option', done => { - graph(anyEntry, undefined, {log: quiet}, error => { + it('calls back with an error when called without `platform` option', async () => { + 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, () => { - expect(resolve).toBeCalledWith(any(String), null, platform, any(Object)); - done(); - }); + await graph(anyEntry, platform, noOpts); + + expect(resolve).toBeCalledWith(any(String), null, platform, any(Object)); }); - 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}, () => { - expect(resolve).toBeCalledWith( - any(String), - null, - any(String), - objectContaining({log}), - ); - done(); - }); + await graph(anyEntry, anyPlatform, {log}); + expect(resolve).toBeCalledWith( + any(String), + null, + any(String), + objectContaining({log}), + ); }); - 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, () => { - expect(load).toBeCalledWith(file1, any(Object)); - expect(load).toBeCalledWith(file2, any(Object)); - done(); - }); + await graph(modules.keys(), anyPlatform, noOpts); + expect(load).toBeCalledWith(file1, any(Object)); + expect(load).toBeCalledWith(file2, any(Object)); }); - it('passes the `optimize` flag on to `load`', done => { - graph(anyEntry, anyPlatform, {optimize: true}, () => { - expect(load).toBeCalledWith( - any(String), - objectContaining({optimize: true}), - ); - done(); - }); + it('passes the `optimize` flag on to `load`', async () => { + await graph(anyEntry, anyPlatform, {optimize: true}); + expect(load).toBeCalledWith( + any(String), + objectContaining({optimize: true}), + ); }); - it('uses `false` as the default for the `optimize` flag', done => { - graph(anyEntry, anyPlatform, noOpts, () => { - expect(load).toBeCalledWith( - any(String), - objectContaining({optimize: false}), - ); - done(); - }); + it('uses `false` as the default for the `optimize` flag', async () => { + await graph(anyEntry, anyPlatform, noOpts); + expect(load).toBeCalledWith( + any(String), + objectContaining({optimize: false}), + ); }); - 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}, () => { - expect(load).toBeCalledWith(any(String), objectContaining({log})); - done(); - }); + await graph(anyEntry, anyPlatform, {log}); + expect(load).toBeCalledWith(any(String), objectContaining({log})); }); - 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, () => { - expect(resolve).toBeCalledWith(id1, entryPath, any(String), any(Object)); - expect(resolve).toBeCalledWith(id2, entryPath, any(String), any(Object)); - done(); - }); + await graph(['entry'], anyPlatform, noOpts); + expect(resolve).toBeCalledWith(id1, entryPath, any(String), any(Object)); + expect(resolve).toBeCalledWith(id2, entryPath, any(String), any(Object)); }); - 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, () => { - expect(resolve).toBeCalledWith(id2, path1, any(String), any(Object)); - expect(load).toBeCalledWith(path1, any(Object)); - expect(load).toBeCalledWith(path2, any(Object)); - done(); - }); + 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)); }); - 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,23 +232,20 @@ describe('Graph:', () => { return {file: createFile('h'), dependencies: []}; }; - graph(['a'], anyPlatform, noOpts, (error, result) => { - expect(error).toEqual(null); - expect(result.modules).toEqual([ - createModule('a', ['b', 'e', 'h']), - createModule('b', ['c', 'd']), - createModule('c'), - createModule('d'), - createModule('e', ['f', 'g']), - createModule('f'), - createModule('g'), - createModule('h'), - ]); - done(); - }); + const result = await graph(['a'], anyPlatform, noOpts); + expect(result.modules).toEqual([ + createModule('a', ['b', 'e', 'h']), + createModule('b', ['c', 'd']), + createModule('c'), + createModule('d'), + createModule('e', ['f', 'g']), + createModule('f'), + createModule('g'), + createModule('h'), + ]); }); - 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) => { - expect(result.entryModules).toEqual([ - createModule('a', ['b']), - createModule('c', ['d']), - ]); - done(); - }); + const result = await graph(['a', 'c'], anyPlatform, noOpts); + expect(result.entryModules).toEqual([ + createModule('a', ['b']), + createModule('c', ['d']), + ]); }); - 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) => { - expect(result.entryModules).toEqual([ - createModule('a', ['b']), - createModule('b', []), - ]); - done(); - }); + const result = await graph(['a', 'b'], anyPlatform, noOpts); + expect(result.entryModules).toEqual([ + createModule('a', ['b']), + createModule('b', []), + ]); }); - 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) => { - expect(error).toEqual(null); - expect(result.modules).toEqual([ - createModule('a', ['b', 'c']), - createModule('b'), - createModule('c'), - createModule('d', ['b', 'c']), - ]); - done(); - }); + const result = await graph(['a', 'd', 'b'], anyPlatform, noOpts); + expect(result.modules).toEqual([ + createModule('a', ['b', 'c']), + createModule('b'), + createModule('c'), + createModule('d', ['b', 'c']), + ]); }); - 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) => { - expect(result.modules).toEqual([ - createModule('a', ['b']), - createModule('b', ['c']), - createModule('c', ['a']), - ]); - done(); - }); + const result = await graph(['a'], anyPlatform, noOpts); + expect(result.modules).toEqual([ + createModule('a', ['b']), + createModule('b', ['c']), + createModule('c', ['a']), + ]); }); - 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) => { - expect(result.modules).toEqual([ - createModule('a', ['b', 'c', 'd']), - createModule('d', []), - ]); - done(); - }); + const result = await graph(['a'], anyPlatform, {skip}); + expect(result.modules).toEqual([ + createModule('a', ['b', 'c', 'd']), + createModule('d', []), + ]); }); }); diff --git a/packages/metro-bundler/src/ModuleGraph/__tests__/ModuleGraph-test.js b/packages/metro-bundler/src/ModuleGraph/__tests__/ModuleGraph-test.js index e7667867..b955064b 100644 --- a/packages/metro-bundler/src/ModuleGraph/__tests__/ModuleGraph-test.js +++ b/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), - }); + }; } diff --git a/packages/metro-bundler/src/ModuleGraph/types.flow.js b/packages/metro-bundler/src/ModuleGraph/types.flow.js index 82a95f47..aef45acd 100644 --- a/packages/metro-bundler/src/ModuleGraph/types.flow.js +++ b/packages/metro-bundler/src/ModuleGraph/types.flow.js @@ -42,8 +42,7 @@ export type GraphFn = ( entryPoints: Iterable, platform: string, options?: ?GraphOptions, - callback?: Callback, -) => void; +) => Promise; type GraphOptions = {| log?: Console,