Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transpile in main thread #390

Merged
merged 1 commit into from
Jan 2, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion api.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,15 @@ var Promise = require('bluebird');
var figures = require('figures');
var globby = require('globby');
var chalk = require('chalk');
var objectAssign = require('object-assign');
var commondir = require('commondir');
var resolveCwd = require('resolve-cwd');
var AvaError = require('./lib/ava-error');
var fork = require('./lib/fork');
var formatter = require('./lib/enhance-assert').formatter();
var CachingPrecompiler = require('./lib/caching-precompiler');
var uniqueTempDir = require('unique-temp-dir');
var findCacheDir = require('find-cache-dir');

function Api(files, options) {
if (!(this instanceof Api)) {
Expand Down Expand Up @@ -44,7 +48,10 @@ util.inherits(Api, EventEmitter);
module.exports = Api;

Api.prototype._runFile = function (file) {
return fork(file, this.options)
var options = objectAssign({}, this.options, {
precompiled: this.precompiler.generateHashForFile(file)
});
return fork(file, options)
.on('stats', this._handleStats)
.on('test', this._handleTest)
.on('unhandledRejections', this._handleRejections)
Expand Down Expand Up @@ -137,6 +144,12 @@ Api.prototype.run = function () {
return Promise.reject(new AvaError('Couldn\'t find any files to test'));
}

var cacheEnabled = self.options.cacheEnabled !== false;
var cacheDir = (cacheEnabled && findCacheDir({name: 'ava', files: files})) ||
uniqueTempDir();
self.options.cacheDir = cacheDir;
self.precompiler = new CachingPrecompiler(cacheDir);

self.fileCount = files.length;

self.base = path.relative('.', commondir('.', files)) + path.sep;
Expand Down
4 changes: 3 additions & 1 deletion cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ var cli = meow([
' --require Module to preload (Can be repeated)',
' --tap Generate TAP output',
' --verbose Enable verbose output',
' --no-cache Disable the transpiler cache',
'',
'Examples',
' ava',
Expand Down Expand Up @@ -77,7 +78,8 @@ if (cli.flags.init) {
var api = new Api(cli.input, {
failFast: cli.flags.failFast,
serial: cli.flags.serial,
require: arrify(cli.flags.require)
require: arrify(cli.flags.require),
cacheEnabled: cli.flags.cache !== false
});

var logger = new Logger();
Expand Down
94 changes: 94 additions & 0 deletions lib/caching-precompiler.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
var cachingTransform = require('caching-transform');
var fs = require('fs');
var path = require('path');
var md5Hex = require('md5-hex');
var stripBom = require('strip-bom');

module.exports = CachingPrecompiler;

function CachingPrecompiler(cacheDir) {
if (!(this instanceof CachingPrecompiler)) {
throw new Error('CachingPrecompiler must be called with new');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamestalmage: What we do everywhere else is just invoke ourself with new and return it. Maybe switch to that?:

if (!(this instanceof CachingPrecompiler)) {
    return new CachingPrecompiler(cacheDir);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure. Probably worth matching style.

That said, after doing it the way you suggest for a long time, I am starting to think it's a mistake. ES2015 classes throw when function called, so magic auto-newing is not future proof. Also, it introduces an easy to overlook failure point when refactoring the function signature. I know I have forgotten to update that statement inside the if condition before.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer being explicit and use new for classes. ES2015 seems to agree with me.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, changed it back to the original behavior (throw if they forget to use new).

}
this.cacheDir = cacheDir;
this.filenameToHash = {};
this.transform = this._createTransform();
}

CachingPrecompiler.prototype._factory = function (cacheDir) {
// This factory method is only called once per process, and only as needed, to defer loading expensive dependencies.
var babel = require('babel-core');
var convertSourceMap = require('convert-source-map');
var presetStage2 = require('babel-preset-stage-2');
var presetES2015 = require('babel-preset-es2015');
var transformRuntime = require('babel-plugin-transform-runtime');

var powerAssert = this._createEspowerPlugin(babel);

function buildOptions(filename, code) {
// Extract existing source maps from the code.
var sourceMap = convertSourceMap.fromSource(code) || convertSourceMap.fromMapFileSource(code, path.dirname(filename));

return {
presets: [presetStage2, presetES2015],
plugins: [powerAssert, transformRuntime],
filename: filename,
sourceMaps: true,
ast: false,
babelrc: false,
inputSourceMap: sourceMap && sourceMap.toObject()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sure to include the babelrc: false thing here.

};
}

return function (code, filename, hash) {
code = code.toString();
var options = buildOptions(filename, code);
var result = babel.transform(code, options);
var mapFile = path.join(cacheDir, hash + '.map');
fs.writeFileSync(mapFile, JSON.stringify(result.map));
return result.code;
};
};

CachingPrecompiler.prototype._createEspowerPlugin = function (babel) {
var createEspowerPlugin = require('babel-plugin-espower/create');
var enhanceAssert = require('./enhance-assert');

// initialize power-assert
return createEspowerPlugin(babel, {
patterns: enhanceAssert.PATTERNS
});
};

CachingPrecompiler.prototype._createTransform = function () {
return cachingTransform({
factory: this._factory.bind(this),
cacheDir: this.cacheDir,
salt: new Buffer(JSON.stringify({
'babel-plugin-espower': require('babel-plugin-espower/package.json').version,
'ava': require('../package.json').version,
'babel-core': require('babel-core/package.json').version
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are just presets, and don't reflect the actual versions of the underlying plugins (they use caret ranges). While it won't break anything to salt with preset versions, it may be providing a false sense of security.

@thejameskyle - Any thoughts on this? Last time I browsed the babel source, it seemed you were only including the version of babel-core in the salt for the cache key.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've replied to that thread: https://phabricator.babeljs.io/T6709#70444

@thejameskyle - Happy to contribute a PR to babel if you want.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's really no good way of caching based on plugins, your solution is full of holes. The current recommendation is for users to wipe out their cache when they change their config.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sindresorhus

Maybe we should default to caching off, and just use the "compile in main thread" optimization. That appears to give us 95% of the benefit anyways.

})),
ext: '.js',
hash: this._hash.bind(this)
});
};

CachingPrecompiler.prototype._hash = function (code, filename, salt) {
var hash = md5Hex([code, filename, salt]);
this.filenameToHash[filename] = hash;
return hash;
};

CachingPrecompiler.prototype.precompileFile = function (filename) {
if (!this.filenameToHash[filename]) {
this.transform(stripBom(fs.readFileSync(filename)), filename);
}
return this.filenameToHash[filename];
};

CachingPrecompiler.prototype.generateHashForFile = function (filename) {
var hash = {};
hash[filename] = this.precompileFile(filename);
return hash;
};
92 changes: 21 additions & 71 deletions lib/test-worker.js
Original file line number Diff line number Diff line change
@@ -1,16 +1,12 @@
'use strict';
var path = require('path');
var fs = require('fs');
var debug = require('debug')('ava');
var pkgDir = require('pkg-dir').sync;
var hasha = require('hasha');
var cacha = require('cacha');
var sourceMapSupport = require('source-map-support');

var opts = JSON.parse(process.argv[2]);
var testPath = opts.file;

var cache = cacha(path.join(pkgDir(path.dirname(testPath)), 'node_modules', '.cache', 'ava'));

if (debug.enabled) {
// Forward the `time-require` `--sorted` flag.
// Intended for internal optimization tests only.
Expand Down Expand Up @@ -38,73 +34,40 @@ sourceMapSupport.install({
if (sourceMapCache[source]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be if (!sourceMapCache[source]) {.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't modify this line, the only modification I made was the one discussed below (saving to disk in a different thread and loading here).

return {
url: source,
map: sourceMapCache[source]
map: fs.readFileSync(sourceMapCache[source], 'utf8')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things:

1. It seems like this should be moved into the compiler.
2.Why are we writing maps to disk just to read them again. Wouldn't it be way faster to just not write them in the first place, and get them from memory here?

EDIT: Never mind. It's a diffrent process. But this feels like the wrong place for this logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where should it go?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I'm not sure. I guess it's fine.

On Wed, Dec 30, 2015 at 7:03 PM James Talmage [email protected]
wrote:

In lib/test-worker.js
#390 (comment):

@@ -34,73 +30,40 @@ sourceMapSupport.install({
if (sourceMapCache[source]) {
return {
url: source,

  •           map: sourceMapCache[source]
    
  •           map: fs.readFileSync(sourceMapCache[source], 'utf8')
    

Where should it go?


Reply to this email directly or view it on GitHub
https://github.com/sindresorhus/ava/pull/390/files#r48645161.

};
}
}
});

var requireFromString = require('require-from-string');
var loudRejection = require('loud-rejection/api')(process);
var serializeError = require('serialize-error');
var send = require('./send');

// if generators are not supported, use regenerator
var options = {
sourceMaps: true
};
var installPrecompiler = require('require-precompiled');
var cacheDir = opts.cacheDir;

// check if test files required ava and show error, when they didn't
exports.avaRequired = false;

// try to load an input source map for the test file, in case the file was
// already compiled once by the user
var inputSourceMap = sourceMapSupport.retrieveSourceMap(testPath);
if (inputSourceMap) {
// source-map-support returns the source map as a json-encoded string, but
// babel requires an actual object
options.inputSourceMap = JSON.parse(inputSourceMap.map);
}

// include test file
var cachePath = hasha(cacheKey(testPath));
var hashPath = cachePath + '_hash';

var prevHash = cache.getSync(hashPath, {encoding: 'utf8'});
var currHash = hasha.fromFileSync(testPath);

if (prevHash === currHash) {
var cached = JSON.parse(cache.getSync(cachePath));

sourceMapCache[testPath] = cached.map;
requireFromString(cached.code, testPath, {
appendPaths: module.paths
});
} else {
var createEspowerPlugin = require('babel-plugin-espower/create');
var babel = require('babel-core');

// initialize power-assert
var powerAssert = createEspowerPlugin(babel, {
patterns: require('./enhance-assert').PATTERNS
});

options.presets = [require('babel-preset-stage-2'), require('babel-preset-es2015')];
options.plugins = [powerAssert, require('babel-plugin-transform-runtime')];
installPrecompiler(function (filename) {
var precompiled = opts.precompiled[filename];
if (precompiled) {
sourceMapCache[filename] = path.join(cacheDir, precompiled + '.map');
return fs.readFileSync(path.join(cacheDir, precompiled + '.js'), 'utf8');
}
return null;
});

var transpiled = babel.transformFileSync(testPath, options);
// Modules need to be able to find `babel-runtime`, which is nested in our node_modules w/ npm@2
var nodeModulesDir = path.join(__dirname, '../node_modules');
var oldNodeModulesPaths = module.constructor._nodeModulePaths;
module.constructor._nodeModulePaths = function () {
var ret = oldNodeModulesPaths.apply(this, arguments);
ret.push(nodeModulesDir);
return ret;
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamestalmage: Why not just modify process.env.NODE_PATH (I think that still works)? Or you could use app-module-path and just pass in the path to node modules.

Or, you know what? This is probably fine. But the bikeshed is to be blue.


cache.setSync(hashPath, currHash);
cache.setSync(cachePath, JSON.stringify({
code: transpiled.code,
map: transpiled.map
}));

sourceMapCache[testPath] = transpiled.map;
requireFromString(transpiled.code, testPath, {
appendPaths: module.paths
});
}
require(testPath);

process.on('uncaughtException', function (exception) {
send('uncaughtException', {exception: serializeError(exception)});
Expand Down Expand Up @@ -151,16 +114,3 @@ process.on('ava-teardown', function () {
function exit() {
send('teardown');
}

function cacheKey(path) {
var key = path;

key += require('../package.json').version;
key += require('babel-core/package.json').version;
key += require('babel-plugin-espower/package.json').version;
key += require('babel-plugin-transform-runtime/package.json').version;
key += require('babel-preset-stage-2/package.json').version;
key += require('babel-preset-es2015/package.json').version;

return hasha(key);
}
12 changes: 8 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -85,44 +85,48 @@
"babel-preset-stage-2": "^6.3.13",
"babel-runtime": "^6.3.19",
"bluebird": "^3.0.0",
"cacha": "^1.0.3",
"caching-transform": "^1.0.0",
"chalk": "^1.0.0",
"co-with-promise": "^4.6.0",
"commondir": "^1.0.1",
"convert-source-map": "^1.1.2",
"core-assert": "^0.1.0",
"debug": "^2.2.0",
"deeper": "^2.1.0",
"empower-core": "^0.2.0",
"figures": "^1.4.0",
"find-cache-dir": "^0.1.1",
"fn-name": "^2.0.0",
"globby": "^4.0.0",
"hasha": "^2.0.2",
"is-generator-fn": "^1.0.0",
"is-observable": "^0.1.0",
"is-promise": "^2.1.0",
"log-update": "^1.0.2",
"loud-rejection": "^1.2.0",
"max-timeout": "^1.0.0",
"md5-hex": "^1.2.0",
"meow": "^3.6.0",
"object-assign": "^4.0.1",
"observable-to-promise": "^0.1.0",
"pkg-dir": "^1.0.0",
"plur": "^2.0.0",
"power-assert-formatter": "^1.3.0",
"power-assert-renderers": "^0.1.0",
"pretty-ms": "^2.0.0",
"require-from-string": "^1.1.0",
"require-precompiled": "^0.1.0",
"resolve-cwd": "^1.0.0",
"serialize-error": "^1.1.0",
"set-immediate-shim": "^1.0.1",
"source-map-support": "^0.4.0",
"strip-bom": "^2.0.0",
"time-require": "^0.1.2",
"unique-temp-dir": "^1.0.0",
"update-notifier": "^0.6.0"
},
"devDependencies": {
"coveralls": "^2.11.4",
"delay": "^1.3.0",
"get-stream": "^1.1.0",
"rimraf": "^2.5.0",
"nyc": "^5.1.0",
"signal-exit": "^2.1.2",
"sinon": "^1.17.2",
Expand Down
Loading