Skip to content

Add support for TypeScript without precompiling #1122

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

Closed
wants to merge 32 commits into from
Closed
Show file tree
Hide file tree
Changes from 23 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
30bf714
Support to Run TypeScript test file(.ts) directly without Precompilin…
huan Nov 15, 2016
0a0e448
make xo happy
huan Nov 15, 2016
c3bf9b3
follow feedbacks from @sindresorhus #1109
huan Nov 21, 2016
7ea3468
add unit test for `--extensions ts` #1109
huan Nov 21, 2016
55ea370
not require TypeScript when no --extension set #1109
huan Nov 21, 2016
2708e40
follow requested changes #1122 #1109
huan Nov 23, 2016
4024791
handle `js` in worker #1122
huan Nov 23, 2016
a925e0e
rename compile to transpile to follow existing names
huan Nov 23, 2016
c0b276a
fix watcher unit tests
huan Nov 23, 2016
47b7dfc
Support to Run TypeScript test file(.ts) directly without Precompilin…
huan Nov 15, 2016
91386f6
make xo happy
huan Nov 15, 2016
d7bfa76
follow feedbacks from @sindresorhus #1109
huan Nov 21, 2016
c76199c
add unit test for `--extensions ts` #1109
huan Nov 21, 2016
c9c1020
not require TypeScript when no --extension set #1109
huan Nov 21, 2016
6ad050a
follow requested changes #1122 #1109
huan Nov 23, 2016
5629601
handle `js` in worker #1122
huan Nov 23, 2016
77f456c
rename compile to transpile to follow existing names
huan Nov 23, 2016
e382848
fix watcher unit tests
huan Nov 23, 2016
5eb9ba1
only require extTs when necessary #1122
huan Nov 23, 2016
685a4fe
merge
huan Nov 23, 2016
5441111
code clean #1122
huan Nov 23, 2016
58690ad
https://github.com/avajs/ava/pull/1122#discussion_r89566725
huan Nov 26, 2016
d91ed25
use [].concat() instead of arrify
huan Nov 26, 2016
6933688
follow @vdemedes https://github.com/avajs/ava/pull/1122#issuecomment-…
huan Nov 28, 2016
accbddb
follow @sindresorhus
huan Nov 29, 2016
ef26c97
follow @novemberborn
huan Dec 4, 2016
aac2fc1
merge
huan Dec 4, 2016
2ea3db7
fix tab/space
huan Dec 4, 2016
8f2bca0
follow @novemberborn
huan Dec 6, 2016
41043e7
get rid of ts-node dev-dependence
huan Dec 6, 2016
6d071e2
follow @novemberborn
huan Dec 10, 2016
d577ff5
upgrade typescript from 2.0 to 2.1
huan Dec 28, 2016
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,4 @@ node_modules
coverage
bench/.results
types/generated.d.ts
yarn.lock
Copy link
Contributor

Choose a reason for hiding this comment

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

Extra change.

Copy link
Author

Choose a reason for hiding this comment

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

deleted.

9 changes: 6 additions & 3 deletions api.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ function Api(options) {
this.options = objectAssign({
cwd: process.cwd(),
resolveTestsFrom: process.cwd(),
match: []
match: [],
extensions: ['js']
}, options);

this.options.require = resolveModules(this.options.require);
Expand All @@ -81,8 +82,9 @@ Api.prototype._runFile = function (file, runStatus, execArgv) {

Api.prototype.run = function (files, options) {
var self = this;
var extensions = (options && options.extensions) || this.options.extensions;

return new AvaFiles({cwd: this.options.resolveTestsFrom, files: files})
return new AvaFiles({cwd: this.options.resolveTestsFrom, files: files, extensions: extensions})
.findTestFiles()
.then(function (files) {
return self._run(files, options);
Expand Down Expand Up @@ -133,7 +135,8 @@ Api.prototype._setupPrecompiler = function (files) {
this.precompiler = new CachingPrecompiler({
path: cacheDir,
babel: this.options.babelConfig,
powerAssert: isPowerAssertEnabled
powerAssert: isPowerAssertEnabled,
extensions: this.options.extensions
});
};

Expand Down
30 changes: 28 additions & 2 deletions lib/caching-precompiler.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ function CachingPrecompiler(options) {

this.babelConfig = babelConfigHelper.validate(options.babel);
this.cacheDirPath = options.path;
this.extensions = options.extensions || ['js'];
this.powerAssert = Boolean(options.powerAssert);
this.fileHashes = {};
this.transform = this._createTransform();
Expand Down Expand Up @@ -47,8 +48,33 @@ CachingPrecompiler.prototype._init = function () {
CachingPrecompiler.prototype._transform = function (code, filePath, hash) {
code = code.toString();

var options = babelConfigHelper.build(this.babelConfig, this.powerAssert, filePath, code);
var result = this.babel.transform(code, options);
var fileExt = path.extname(filePath).replace(/^\./, '');
Copy link
Member

Choose a reason for hiding this comment

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

path.extname(filePath).slice(1) seems cleaner.

Copy link
Author

Choose a reason for hiding this comment

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

done.

var isCustomExt = this.extensions.filter(function (ext) {
return (ext !== 'js' && ext === fileExt);
}).length > 0;
Copy link
Member

Choose a reason for hiding this comment

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

This could be simplified to:

var isCustomExt = fileExt !== 'js' && this.extensions.indexOf(fileExt) > -1

Copy link
Author

Choose a reason for hiding this comment

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

done.


var result;

if (isCustomExt) {
switch (fileExt) {
Copy link
Contributor

@vadimdemedes vadimdemedes Nov 27, 2016

Choose a reason for hiding this comment

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

I'm not a fan of switch statements, simple if here could do just as well. @sindresorhus ?

Copy link
Member

Choose a reason for hiding this comment

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

@vdemedes Agreed. I usually don't use switch unless there are lots of same if's.

Copy link
Member

Choose a reason for hiding this comment

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

@zixia could you change this to an if statement?

Copy link
Author

@huan huan Dec 6, 2016

Choose a reason for hiding this comment

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

ok. done.

Now we only have one extension: ts. If we support more extensions in the future, there might come with 'lots of same ifs'

Copy link

Choose a reason for hiding this comment

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

What about .tsx for TypeScript/React?

Copy link

Choose a reason for hiding this comment

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

Could we do the following to cover both .ts and .tsx files?

if (/^tsx?$/.test(fileExt))

Copy link
Author

Choose a reason for hiding this comment

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

I use .ts only and I do not know anything about react(I use angular).

Could TypeScript transpile .tsx by default?

Copy link

Choose a reason for hiding this comment

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

With default settings – no. But with the --jsx preserve or --jsx react compiler options – yes. And even test files commonly use the .tsx extension to mock components.

case 'ts':
var extTs = require('./ext/ts');
result = extTs.transpile(code, filePath);
break;

/**
* here for other extensions in the future
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

removed.

*/

default:
throw new Error('`--extension ' + fileExt + '` is not supported by AVA.');
Copy link
Member

Choose a reason for hiding this comment

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

Let's check for allowed extensions in lib/cli instead.

Copy link
Author

Choose a reason for hiding this comment

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

Had changed to if.

Where are the allowed extensions in cli.js? Did you mean we should save all supported extensions to an array in cli.js, then reference it in other js files?

}
}

if (!result) {
Copy link
Member

Choose a reason for hiding this comment

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

Should just be an else, assuming the body for if (isCustomExt) assigns result or throws.

Copy link
Author

Choose a reason for hiding this comment

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

done.

var options = babelConfigHelper.build(this.babelConfig, this.powerAssert, filePath, code);
result = this.babel.transform(code, options);
}

// save source map
var mapPath = path.join(this.cacheDirPath, hash + '.js.map');
Expand Down
14 changes: 10 additions & 4 deletions lib/cli.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ exports.run = function () {
' --source, -S Pattern to match source files so tests can be re-run (Can be repeated)',
' --timeout, -T Set global timeout',
' --concurrency, -c Maximum number of test files running at the same time (EXPERIMENTAL)',
' --extension, -e Test file extension, for example `ts` for TypeScript (Can be repeated)',
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 example here is extra. Or it could at least be shortened to:

Test file extension (e.g. `coffee`, `ts`)

Copy link
Author

Choose a reason for hiding this comment

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

shorted.

Copy link
Member

Choose a reason for hiding this comment

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

I agree it could be shortened, but not like that.

Should be:

Test file extension (e.g. `ts`) (Can be repeated)

Copy link
Author

Choose a reason for hiding this comment

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

done.

'',
'Examples',
' ava',
Expand All @@ -60,7 +61,8 @@ exports.run = function () {
'timeout',
'source',
'match',
'concurrency'
'concurrency',
'extension'
],
boolean: [
'fail-fast',
Expand All @@ -78,7 +80,8 @@ exports.run = function () {
w: 'watch',
S: 'source',
T: 'timeout',
c: 'concurrency'
c: 'concurrency',
e: 'extension'
}
});

Expand All @@ -100,6 +103,8 @@ exports.run = function () {
throw new Error(colors.error(figures.cross) + ' The --require and -r flags are deprecated. Requirements should be configured in package.json - see documentation.');
}

var extensionList = [].concat(cli.flags.extension || 'js');
Copy link
Contributor

Choose a reason for hiding this comment

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

extensions might be a better name.

Copy link
Author

Choose a reason for hiding this comment

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

I use extensions at first. I try to make the name more clear by changed it to extensionList .

However, I had followed your suggestion, renamed it back to extensions.


var api = new Api({
failFast: cli.flags.failFast,
serial: cli.flags.serial,
Expand All @@ -112,7 +117,8 @@ exports.run = function () {
resolveTestsFrom: cli.input.length === 0 ? pkgDir : process.cwd(),
pkgDir: pkgDir,
timeout: cli.flags.timeout,
concurrency: cli.flags.concurrency ? parseInt(cli.flags.concurrency, 10) : 0
concurrency: cli.flags.concurrency ? parseInt(cli.flags.concurrency, 10) : 0,
extensions: extensionList
});

var reporter;
Expand Down Expand Up @@ -143,7 +149,7 @@ exports.run = function () {

if (cli.flags.watch) {
try {
var watcher = new Watcher(logger, api, files, arrify(cli.flags.source));
var watcher = new Watcher(logger, api, files, arrify(cli.flags.source), extensionList);
watcher.observeStdin(process.stdin);
} catch (err) {
if (err.name === 'AvaError') {
Expand Down
111 changes: 111 additions & 0 deletions lib/ext/ts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
'use strict';
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

removed.

* AVA File Extension for TypeScript
* Author: https://github.com/zixia
*/
var fs = require('fs');

var typescript;
try {
typescript = require('typescript');
} catch (err) {
console.log('AVA error: `--extensions=ts` require TypeScript to be installed.');
Copy link
Member

@sindresorhus sindresorhus Nov 29, 2016

Choose a reason for hiding this comment

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

Use console.error

AVA error: => AVA:

Same with the below code.

Copy link
Author

Choose a reason for hiding this comment

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

done.

throw err;
Copy link
Member

Choose a reason for hiding this comment

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

Rather than logging and then rethrowing the original error construct a new error like in

ava/lib/cli.js

Line 99 in 0f04001

throw new Error(colors.error(figures.cross) + ' The TAP reporter is not available when using watch mode.');
and throw that. It'll be logged correctly.

Copy link
Author

Choose a reason for hiding this comment

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

done.

}
var tsnode; // = require('ts-node'); XXX: maybe useful in the future, because ts-node do more than typescript for our case.
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment should be removed.

Copy link
Author

Choose a reason for hiding this comment

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

removed.


var typescriptOptions = {
module: typescript.ModuleKind.CommonJS,
target: typescript.ScriptTarget.ES6,
sourceMap: true,
inlineSources: true,
inlineSourceMap: true
};

/**
*
* Ts-Node version
*
*/
Copy link
Member

Choose a reason for hiding this comment

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

Remove comment.

Copy link
Author

Choose a reason for hiding this comment

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

removed

// const originalJsHandler = require.extensions['.js']
// const tsService = require('ts-node').register()
// const tsnodeTsHandler = require.extensions['.ts']
// const tsnodeTsxHandler = require.extensions['.tsx']
// delete require.extensions['.ts']
// delete require.extensions['.tsx']

function tsnodeRegister() {
tsnode.register(typescriptOptions);
// if (require.extensions['.ts']) {
// throw new Error('there has already a register for `ts`')
// }
// if (!tsService) {
// throw new Error('no ts service')
// }
// require.extensions['.ts'] = tsnodeTsHandler
// require.extensions['.tsx'] = tsnodeTsxHandler
}

function tsnodeTranspile(code, fileName) {
const tsService = require('ts-node').register(typescriptOptions);

if (!tsService) {
throw new Error('no ts service');
}

const result = tsService().compile(code, fileName);
// console.log(result)
return result;
}

/**
*
* Typescript version
*
*/

function typescriptRegister() {
require.extensions['.ts'] = function (module, fileName) {
var code = fs.readFileSync(fileName, 'utf8');
var result = typescript.transpileModule(code, {
fileName,
compilerOptions: typescriptOptions,
reportDiagnostics: true
});
// console.log('\n\n\n')
// console.log(result.outputText)
// console.log('\n\n\n')
// process.exit(0)
module._compile(result.outputText, fileName);
};

require('source-map-support').install({
hookRequire: true
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't ts-node already do this?

Copy link
Author

Choose a reason for hiding this comment

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

yes and no.

ts-node will do this if we use transpile function from ts-node. However, the current version of lib/ext/ts.js is not using the ts-node by default, it use the raw typescript module, so we have to do this by ourselves.

I leave the ts-node in lib/ext/ts.js is because at the beginning I'm not sure which one is better, so I implement both, but at last I decided to use pure typescript by default:

module.exports = module.exports.default = {
  register: typescriptRegister,
  transpile: typescriptTranspile,

Do you think we should use ts-node, or I need to get rid of the additional ts-node code in lib/ext/ts.js?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like ts-node is closer to babel-node than babel-core (and ts-node/register is equivalent to babel-core/register). IMO this module should just use typescript to transpile the code, and no other functionality is required.

For consistency we should not automatically transpile source files, at least until we have a better answer for the .js use case.

Copy link
Member

Choose a reason for hiding this comment

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

@zixia per the above discusison, could you simplify this module to just the transpile function?

Copy link
Author

Choose a reason for hiding this comment

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

Hi @novemberborn ,

I had simplified this module, removed all the ts-node parts, cleaned the code, but not just the transpile.

The register() function has to be kept because if we run test written in TypeScript, the lib/test-worker.js must has the ability to transpile TypeScript, which is enabled by extTs.register() at here: https://github.com/zixia/ava/blob/typescript/lib/test-worker.js#L38


function typescriptTranspile(code, fileName) {
var result = typescript.transpileModule(code, {
fileName,
compilerOptions: typescriptOptions,
reportDiagnostics: true
});
// console.log(result)
return {
code: result.outputText,
map: result.sourceMapText
};
}

var extTs = {
Copy link
Member

Choose a reason for hiding this comment

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

Just export this directly.

Copy link
Author

Choose a reason for hiding this comment

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

done.

register: typescriptRegister,
transpile: typescriptTranspile,

typescriptRegister,
typescriptTranspile,

tsnodeRegister,
tsnodeTranspile
};

module.exports = extTs;
17 changes: 17 additions & 0 deletions lib/test-worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,23 @@ process.installPrecompilerHook();
const dependencies = [];
process.installDependencyTracking(dependencies, testPath);

if (opts.extensions) {
opts.extensions.forEach(function (ext) {
switch (ext) {
case 'ts':
var extTs = require('./ext/ts');
extTs.register();
break;
case 'js':
// We already have babel for ES6/7
break;

default:
throw new Error('`--extension ' + ext + '` is not supported by AVA.');
Copy link
Member

Choose a reason for hiding this comment

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

--extension ' => --extension='

Copy link
Author

Choose a reason for hiding this comment

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

done.

}
});
}
Copy link
Member

Choose a reason for hiding this comment

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

Please remove these changes. Right now I'd prefer our TypeScript support is consistent with our Babel support, where dependencies are not automatically transpiled.

I know that's not ideal but we can revisit it later.

Copy link
Author

Choose a reason for hiding this comment

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

I don't think the TypeScript support would still work after we removed those changes. Because if no extTs.register(), then test-worker.js will not work with .ts test files.

This TypeScript support will only be enabled when we specify --extension ts arg with AVA, which is the case we need to automatically transpile all TypeScript file with extension: .ts

And about the dependencies, I think for TypeScript is different as for JavaScript, because:

  1. There are almost no TypeScript dependencies in NPM(node_modules)
  2. The .ts extension is not the default JavaScript file extension, so it's almost only in our development environment.

Please correct me if I'm wrong, or missed anything. Thanks.


require(testPath); // eslint-disable-line import/no-dynamic-require

process.on('unhandledRejection', throwsHelper);
Expand Down
6 changes: 4 additions & 2 deletions lib/watcher.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,11 @@ function rethrowAsync(err) {
});
}

function Watcher(logger, api, files, sources) {
function Watcher(logger, api, files, sources, extensions) {
this.debouncer = new Debouncer(this);
this.avaFiles = new AvaFiles({
files: files,
extensions: extensions,
sources: sources
});

Expand Down Expand Up @@ -59,7 +60,8 @@ function Watcher(logger, api, files, sources) {

var self = this;
this.busy = api.run(specificFiles || files, {
runOnlyExclusive: runOnlyExclusive
runOnlyExclusive: runOnlyExclusive,
extensions: extensions
Copy link
Member

Choose a reason for hiding this comment

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

Needs a watcher test for TypeScript.

}).then(function (runStatus) {
runStatus.previousFailCount = self.sumPreviousFailures(currentVector);
logger.finish(runStatus);
Expand Down
2 changes: 2 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,8 @@
"source-map-fixtures": "^2.1.0",
"tap": "^8.0.0",
"touch": "^1.0.0",
"ts-node": "^1.6.0",
"typescript": "^2.0.3",
"xo": "^0.17.0",
"zen-observable": "^0.3.0"
},
Expand Down
23 changes: 23 additions & 0 deletions test/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,29 @@ test('Without Pool: test files can be forced to run in exclusive mode', function
});
});

test('TypeScript default not supported', function (t) {
var api = new Api();

return api.run(
[path.join(__dirname, 'fixture/typescript/simple.ts')]
).then(function (result) {
t.fail('should not get result here: ' + result);
}).catch(function (err) {
t.ok('should not found ts file by default, get err: ' + err);
});
});

test('TypeScript supported by set `extensions`', function (t) {
var api = new Api({extensions: ['ts']});
return api.run(
[path.join(__dirname, 'fixture/typescript/simple.ts')]
).then(function (result) {
t.is(result.testCount, 1);
t.is(result.passCount, 1);
t.is(result.failCount, 0);
});
});
Copy link
Member

Choose a reason for hiding this comment

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

Needs a test to ensure it support reading user TS config file.


generateTests('With Pool: ', function (options) {
options = options || {};
options.concurrency = 2;
Expand Down
6 changes: 6 additions & 0 deletions test/fixture/typescript/simple.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
import { test } from 'ava';

test('AVA run TypeScript without tsc', t => {
let i: number = 42;
t.is(i, <number>42, 'meaning of life');
});
Loading