`LoadFn`/`ResolveFn` without callbacks

Summary:
Changes `LoadFn` to be synchronous or return a promise, and `ResolveFn` to be synchronsous.

This makes for a nicer API, without losing the property of not yielding to the event loop for synchronous work.

Reviewed By: jeanlauliac

Differential Revision: D5855963

fbshipit-source-id: 4b3c3363f4e49a9890586462925e8e400872feb2
This commit is contained in:
David Aurelio 2017-09-19 09:18:50 -07:00 committed by Facebook Github Bot
parent 3f2b200e01
commit 6062a9bd34
3 changed files with 97 additions and 79 deletions

View File

@ -10,6 +10,7 @@
*/ */
'use strict'; 'use strict';
const asyncify = require('async/asyncify');
const emptyFunction = require('fbjs/lib/emptyFunction'); const emptyFunction = require('fbjs/lib/emptyFunction');
const invariant = require('fbjs/lib/invariant'); const invariant = require('fbjs/lib/invariant');
const memoize = require('async/memoize'); const memoize = require('async/memoize');
@ -23,7 +24,7 @@ import type {
File, File,
GraphFn, GraphFn,
LoadFn, LoadFn,
ResolveFnCallback, ResolveFn,
} from './types.flow'; } from './types.flow';
type Async$Queue<T, C> = { type Async$Queue<T, C> = {
@ -52,7 +53,10 @@ type LoadQueue =
const NO_OPTIONS = {}; const NO_OPTIONS = {};
exports.create = function create(resolve: ResolveFnCallback, load: LoadFn): GraphFn { 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, callback = emptyFunction) {
const { const {
log = (console: any), log = (console: any),
@ -67,8 +71,9 @@ exports.create = function create(resolve: ResolveFnCallback, load: LoadFn): Grap
} }
const loadQueue: LoadQueue = queue(seq( const loadQueue: LoadQueue = queue(seq(
({id, parent}, cb) => resolve(id, parent, platform, options || NO_OPTIONS, cb), ({id, parent}, cb) => resolveCallback(id, parent, platform, options || NO_OPTIONS, cb),
memoize((file, cb) => load(file, {log, optimize}, cb)), memoize((file, cb) => loadCallback(file, {log, optimize}, cb)),
({file, dependencies}, cb) => cb(null, file, dependencies),
), Number.MAX_SAFE_INTEGER); ), Number.MAX_SAFE_INTEGER);
const {collect, loadModule} = createGraphHelpers(loadQueue, skip); const {collect, loadModule} = createGraphHelpers(loadQueue, skip);

View File

@ -28,8 +28,8 @@ describe('Graph:', () => {
beforeEach(() => { beforeEach(() => {
load = fn(); load = fn();
resolve = fn(); resolve = fn();
resolve.stub.yields(null, 'arbitrary file'); resolve.stub.returns('arbitrary file');
load.stub.yields(null, createFile('arbitrary file'), []); load.stub.returns({file: createFile('arbitrary file'), dependencies: []});
graph = Graph.create(resolve, load); graph = Graph.create(resolve, load);
}); });
@ -45,7 +45,7 @@ describe('Graph:', () => {
const entryPoint = '/arbitrary/path'; const entryPoint = '/arbitrary/path';
graph([entryPoint], anyPlatform, noOpts, () => { graph([entryPoint], anyPlatform, noOpts, () => {
expect(resolve).toBeCalledWith( expect(resolve).toBeCalledWith(
entryPoint, null, any(String), any(Object), any(Function)); entryPoint, null, any(String), any(Object));
done(); done();
}); });
}); });
@ -54,9 +54,9 @@ describe('Graph:', () => {
const entryPoints = ['Arbitrary', '../entry.js']; const entryPoints = ['Arbitrary', '../entry.js'];
graph(entryPoints, anyPlatform, noOpts, () => { graph(entryPoints, anyPlatform, noOpts, () => {
expect(resolve).toBeCalledWith( expect(resolve).toBeCalledWith(
entryPoints[0], null, any(String), any(Object), any(Function)); entryPoints[0], null, any(String), any(Object));
expect(resolve).toBeCalledWith( expect(resolve).toBeCalledWith(
entryPoints[1], null, any(String), any(Object), any(Function)); entryPoints[1], null, any(String), any(Object));
done(); done();
}); });
@ -73,7 +73,7 @@ describe('Graph:', () => {
const platform = 'any'; const platform = 'any';
graph(anyEntry, platform, noOpts, () => { graph(anyEntry, platform, noOpts, () => {
expect(resolve).toBeCalledWith( expect(resolve).toBeCalledWith(
any(String), null, platform, any(Object), any(Function)); any(String), null, platform, any(Object));
done(); done();
}); });
}); });
@ -82,14 +82,14 @@ describe('Graph:', () => {
const log = new Console(); const log = new Console();
graph(anyEntry, anyPlatform, {log}, () => { graph(anyEntry, anyPlatform, {log}, () => {
expect(resolve).toBeCalledWith( expect(resolve).toBeCalledWith(
any(String), null, any(String), objectContaining({log}), any(Function)); any(String), null, any(String), objectContaining({log}));
done(); done();
}); });
}); });
it('calls back with every error produced by `resolve`', done => { it('calls back with every error produced by `resolve`', done => {
const error = Error(); const error = Error();
resolve.stub.yields(error); resolve.stub.throws(error);
graph(anyEntry, anyPlatform, noOpts, e => { graph(anyEntry, anyPlatform, noOpts, e => {
expect(e).toBe(error); expect(e).toBe(error);
done(); done();
@ -97,10 +97,13 @@ describe('Graph:', () => {
}); });
it('only calls back once if two parallel invocations of `resolve` fail', done => { it('only calls back once if two parallel invocations of `resolve` fail', done => {
load.stub.yields(null, createFile('with two deps'), ['depA', 'depB']); load.stub.returns({
file: createFile('with two deps'),
dependencies: ['depA', 'depB']},
);
resolve.stub resolve.stub
.withArgs('depA').yieldsAsync(new Error()) .withArgs('depA').throws(new Error())
.withArgs('depB').yieldsAsync(new Error()); .withArgs('depB').throws(new Error());
let calls = 0; let calls = 0;
function callback() { function callback() {
@ -122,13 +125,13 @@ describe('Graph:', () => {
['../entry.js', '/whereever/is/entry.js'], ['../entry.js', '/whereever/is/entry.js'],
]); ]);
for (const [id, file] of modules) { for (const [id, file] of modules) {
resolve.stub.withArgs(id).yields(null, file); resolve.stub.withArgs(id).returns(file);
} }
const [file1, file2] = modules.values(); const [file1, file2] = modules.values();
graph(modules.keys(), anyPlatform, noOpts, () => { graph(modules.keys(), anyPlatform, noOpts, () => {
expect(load).toBeCalledWith(file1, any(Object), any(Function)); expect(load).toBeCalledWith(file1, any(Object));
expect(load).toBeCalledWith(file2, any(Object), any(Function)); expect(load).toBeCalledWith(file2, any(Object));
done(); done();
}); });
}); });
@ -136,7 +139,7 @@ describe('Graph:', () => {
it('passes the `optimize` flag on to `load`', done => { it('passes the `optimize` flag on to `load`', done => {
graph(anyEntry, anyPlatform, {optimize: true}, () => { graph(anyEntry, anyPlatform, {optimize: true}, () => {
expect(load).toBeCalledWith( expect(load).toBeCalledWith(
any(String), objectContaining({optimize: true}), any(Function)); any(String), objectContaining({optimize: true}));
done(); done();
}); });
}); });
@ -144,7 +147,7 @@ describe('Graph:', () => {
it('uses `false` as the default for the `optimize` flag', done => { it('uses `false` as the default for the `optimize` flag', done => {
graph(anyEntry, anyPlatform, noOpts, () => { graph(anyEntry, anyPlatform, noOpts, () => {
expect(load).toBeCalledWith( expect(load).toBeCalledWith(
any(String), objectContaining({optimize: false}), any(Function)); any(String), objectContaining({optimize: false}));
done(); done();
}); });
}); });
@ -153,14 +156,14 @@ describe('Graph:', () => {
const log = new Console(); const log = new Console();
graph(anyEntry, anyPlatform, {log}, () => { graph(anyEntry, anyPlatform, {log}, () => {
expect(load) expect(load)
.toBeCalledWith(any(String), objectContaining({log}), any(Function)); .toBeCalledWith(any(String), objectContaining({log}));
done(); done();
}); });
}); });
it('calls back with every error produced by `load`', done => { it('calls back with every error produced by `load`', done => {
const error = Error(); const error = Error();
load.stub.yields(error); load.stub.throws(error);
graph(anyEntry, anyPlatform, noOpts, e => { graph(anyEntry, anyPlatform, noOpts, e => {
expect(e).toBe(error); expect(e).toBe(error);
done(); done();
@ -171,15 +174,17 @@ describe('Graph:', () => {
const entryPath = '/path/to/entry.js'; const entryPath = '/path/to/entry.js';
const id1 = 'required/id'; const id1 = 'required/id';
const id2 = './relative/import'; const id2 = './relative/import';
resolve.stub.withArgs('entry').yields(null, entryPath); resolve.stub.withArgs('entry').returns(entryPath);
load.stub.withArgs(entryPath) load.stub.withArgs(entryPath).returns({
.yields(null, {path: entryPath}, [id1, id2]); file: {path: entryPath},
dependencies: [id1, id2],
});
graph(['entry'], anyPlatform, noOpts, () => { graph(['entry'], anyPlatform, noOpts, () => {
expect(resolve).toBeCalledWith( expect(resolve).toBeCalledWith(
id1, entryPath, any(String), any(Object), any(Function)); id1, entryPath, any(String), any(Object));
expect(resolve).toBeCalledWith( expect(resolve).toBeCalledWith(
id2, entryPath, any(String), any(Object), any(Function)); id2, entryPath, any(String), any(Object));
done(); done();
}); });
}); });
@ -192,22 +197,22 @@ describe('Graph:', () => {
const path2 = '/path/to/dep/2'; const path2 = '/path/to/dep/2';
resolve.stub resolve.stub
.withArgs(id1).yields(null, path1) .withArgs(id1).returns(path1)
.withArgs(id2).yields(null, path2) .withArgs(id2).returns(path2)
.withArgs('entry').yields(null, entryPath); .withArgs('entry').returns(entryPath);
load.stub load.stub
.withArgs(entryPath).yields(null, {path: entryPath}, [id1]) .withArgs(entryPath).returns({file: {path: entryPath}, dependencies: [id1]})
.withArgs(path1).yields(null, {path: path1}, [id2]); .withArgs(path1).returns({file: {path: path1}, dependencies: [id2]});
graph(['entry'], anyPlatform, noOpts, () => { graph(['entry'], anyPlatform, noOpts, () => {
expect(resolve).toBeCalledWith(id2, path1, any(String), any(Object), any(Function)); expect(resolve).toBeCalledWith(id2, path1, any(String), any(Object));
expect(load).toBeCalledWith(path1, any(Object), any(Function)); expect(load).toBeCalledWith(path1, any(Object));
expect(load).toBeCalledWith(path2, any(Object), any(Function)); expect(load).toBeCalledWith(path2, any(Object));
done(); done();
}); });
}); });
it('resolves modules in depth-first traversal order, regardless of the order of resolution', it('resolves modules in depth-first traversal order, regardless of the order of loading',
done => { done => {
load.stub.reset(); load.stub.reset();
resolve.stub.reset(); resolve.stub.reset();
@ -222,20 +227,26 @@ describe('Graph:', () => {
]; ];
ids.forEach(id => { ids.forEach(id => {
const path = idToPath(id); const path = idToPath(id);
resolve.stub.withArgs(id).yields(null, path); resolve.stub.withArgs(id).returns(path);
load.stub.withArgs(path).yields(null, createFile(id), []); load.stub.withArgs(path).returns({file: createFile(id), dependencies: []});
}); });
load.stub.withArgs(idToPath('a')).yields(null, createFile('a'), ['b', 'e', 'h']); load.stub.withArgs(idToPath('a')).returns({file: createFile('a'), dependencies: ['b', 'e', 'h']});
load.stub.withArgs(idToPath('b')).yields(null, createFile('b'), ['c', 'd']);
load.stub.withArgs(idToPath('e')).yields(null, createFile('e'), ['f', 'g']);
// load certain ids later // load certain files later
['b', 'e', 'h'].forEach(id => resolve.stub.withArgs(id).resetBehavior()); const b = deferred({file: createFile('b'), dependencies: ['c', 'd']});
resolve.stub.withArgs('h').func = (a, b, c, d, callback) => { const e = deferred({file: createFile('e'), dependencies: ['f', 'g']});
callback(null, idToPath('h')); load.stub
['e', 'b'].forEach( .withArgs(idToPath('b')).returns(b.promise)
id => resolve.stub.withArgs(id).yield(null, idToPath(id))); .withArgs(idToPath('e')).returns(e.promise)
}; .withArgs(idToPath('h')).func = (f, o) => {
process.nextTick(() => {
// `e` loads after `h`
e.resolve();
// `b` loads after `a`
process.nextTick(b.resolve);
});
return {file: createFile('h'), dependencies: []};
};
graph(['a'], anyPlatform, noOpts, (error, result) => { graph(['a'], anyPlatform, noOpts, (error, result) => {
expect(error).toEqual(null); expect(error).toEqual(null);
@ -258,13 +269,13 @@ describe('Graph:', () => {
load.stub.reset(); load.stub.reset();
resolve.stub.reset(); resolve.stub.reset();
load.stub.withArgs(idToPath('a')).yields(null, createFile('a'), ['b']); load.stub.withArgs(idToPath('a')).returns({file: createFile('a'), dependencies: ['b']});
load.stub.withArgs(idToPath('b')).yields(null, createFile('b'), []); load.stub.withArgs(idToPath('b')).returns({file: createFile('b'), dependencies: []});
load.stub.withArgs(idToPath('c')).yields(null, createFile('c'), ['d']); load.stub.withArgs(idToPath('c')).returns({file: createFile('c'), dependencies: ['d']});
load.stub.withArgs(idToPath('d')).yields(null, createFile('d'), []); load.stub.withArgs(idToPath('d')).returns({file: createFile('d'), dependencies: []});
'abcd'.split('') 'abcd'.split('')
.forEach(id => resolve.stub.withArgs(id).yields(null, idToPath(id))); .forEach(id => resolve.stub.withArgs(id).returns(idToPath(id)));
graph(['a', 'c'], anyPlatform, noOpts, (error, result) => { graph(['a', 'c'], anyPlatform, noOpts, (error, result) => {
expect(result.entryModules).toEqual([ expect(result.entryModules).toEqual([
@ -279,11 +290,11 @@ describe('Graph:', () => {
load.stub.reset(); load.stub.reset();
resolve.stub.reset(); resolve.stub.reset();
load.stub.withArgs(idToPath('a')).yields(null, createFile('a'), ['b']); load.stub.withArgs(idToPath('a')).returns({file: createFile('a'), dependencies: ['b']});
load.stub.withArgs(idToPath('b')).yields(null, createFile('b'), []); load.stub.withArgs(idToPath('b')).returns({file: createFile('b'), dependencies: []});
'ab'.split('') 'ab'.split('')
.forEach(id => resolve.stub.withArgs(id).yields(null, idToPath(id))); .forEach(id => resolve.stub.withArgs(id).returns(idToPath(id)));
graph(['a', 'b'], anyPlatform, noOpts, (error, result) => { graph(['a', 'b'], anyPlatform, noOpts, (error, result) => {
expect(result.entryModules).toEqual([ expect(result.entryModules).toEqual([
@ -298,12 +309,12 @@ describe('Graph:', () => {
const ids = ['a', 'b', 'c', 'd']; const ids = ['a', 'b', 'c', 'd'];
ids.forEach(id => { ids.forEach(id => {
const path = idToPath(id); const path = idToPath(id);
resolve.stub.withArgs(id).yields(null, path); resolve.stub.withArgs(id).returns(path);
load.stub.withArgs(path).yields(null, createFile(id), []); load.stub.withArgs(path).returns({file: createFile(id), dependencies: []});
}); });
['a', 'd'].forEach(id => ['a', 'd'].forEach(id =>
load.stub load.stub
.withArgs(idToPath(id)).yields(null, createFile(id), ['b', 'c'])); .withArgs(idToPath(id)).returns({file: createFile(id), dependencies: ['b', 'c']}));
graph(['a', 'd', 'b'], anyPlatform, noOpts, (error, result) => { graph(['a', 'd', 'b'], anyPlatform, noOpts, (error, result) => {
expect(error).toEqual(null); expect(error).toEqual(null);
@ -319,13 +330,13 @@ describe('Graph:', () => {
it('handles dependency cycles', done => { it('handles dependency cycles', done => {
resolve.stub resolve.stub
.withArgs('a').yields(null, idToPath('a')) .withArgs('a').returns(idToPath('a'))
.withArgs('b').yields(null, idToPath('b')) .withArgs('b').returns(idToPath('b'))
.withArgs('c').yields(null, idToPath('c')); .withArgs('c').returns(idToPath('c'));
load.stub load.stub
.withArgs(idToPath('a')).yields(null, createFile('a'), ['b']) .withArgs(idToPath('a')).returns({file: createFile('a'), dependencies: ['b']})
.withArgs(idToPath('b')).yields(null, createFile('b'), ['c']) .withArgs(idToPath('b')).returns({file: createFile('b'), dependencies: ['c']})
.withArgs(idToPath('c')).yields(null, createFile('c'), ['a']); .withArgs(idToPath('c')).returns({file: createFile('c'), dependencies: ['a']});
graph(['a'], anyPlatform, noOpts, (error, result) => { graph(['a'], anyPlatform, noOpts, (error, result) => {
expect(result.modules).toEqual([ expect(result.modules).toEqual([
@ -339,12 +350,12 @@ describe('Graph:', () => {
it('can skip files', done => { it('can skip files', done => {
['a', 'b', 'c', 'd', 'e'].forEach( ['a', 'b', 'c', 'd', 'e'].forEach(
id => resolve.stub.withArgs(id).yields(null, idToPath(id))); id => resolve.stub.withArgs(id).returns(idToPath(id)));
load.stub load.stub
.withArgs(idToPath('a')).yields(null, createFile('a'), ['b', 'c', 'd']) .withArgs(idToPath('a')).returns({file: createFile('a'), dependencies: ['b', 'c', 'd']})
.withArgs(idToPath('b')).yields(null, createFile('b'), ['e']); .withArgs(idToPath('b')).returns({file: createFile('b'), dependencies: ['e']});
['c', 'd', 'e'].forEach(id => ['c', 'd', 'e'].forEach(id =>
load.stub.withArgs(idToPath(id)).yields(null, createFile(id), [])); load.stub.withArgs(idToPath(id)).returns({file: createFile(id), dependencies: []}));
const skip = new Set([idToPath('b'), idToPath('c')]); const skip = new Set([idToPath('b'), idToPath('c')]);
graph(['a'], anyPlatform, {skip}, (error, result) => { graph(['a'], anyPlatform, {skip}, (error, result) => {
@ -375,3 +386,9 @@ function createModule(id, dependencies = []): Module {
function idToPath(id) { function idToPath(id) {
return '/path/to/' + id; return '/path/to/' + id;
} }
function deferred(value) {
let resolve;
const promise = new Promise(res => resolve = res);
return {promise, resolve: () => resolve(value)};
}

View File

@ -58,11 +58,15 @@ export type GraphResult = {|
export type IdForPathFn = {path: string} => number; export type IdForPathFn = {path: string} => number;
type LoadResult = {
file: File,
dependencies: Array<string>,
};
export type LoadFn = ( export type LoadFn = (
file: string, file: string,
options: LoadOptions, options: LoadOptions,
callback: Callback<File, Array<string>>, ) => LoadResult | Promise<LoadResult>;
) => void;
type LoadOptions = {| type LoadOptions = {|
log?: Console, log?: Console,
@ -108,14 +112,6 @@ export type ResolveFn = (
options?: ResolveOptions, options?: ResolveOptions,
) => string; ) => string;
export type ResolveFnCallback = (
id: string,
source: ?string,
platform: string,
options?: ResolveOptions,
callback: Callback<string>,
) => void;
type ResolveOptions = { type ResolveOptions = {
log?: Console, log?: Console,
}; };