Add support for TypeScript without precompiling #1122

huan · 2016-11-21T08:29:50Z

Notice that this PR has to work together with avajs/ava-files#9

So unit testing in CI will expect failure before we merge&update AvaFiles NPM.

In my development environment with npm link ava-files, all unit tests had passed.

sindresorhus · 2016-11-22T18:44:53Z

package.json

    "xo": "^0.17.0",
    "zen-observable": "^0.3.0"
  },
  "xo": {
    "rules": {
      "import/newline-after-import": "off",
-      "no-use-extend-native/no-use-extend-native": "off"
+      "no-use-extend-native/no-use-extend-native": "off",
+      "no-useless-escape": "off"


Fix the issue instead.

Ok. fixed.

but this part looks to be a little weird:

var isBabelPath = /^babel-runtime[\\/]?/.test(path.node.value);

Ref: http://stackoverflow.com/a/40315390/1123955

And a question: How did the AVA pass the CI test? I just clone the AVA repo, run npm install && npm t, XO will throw no-useless-escape error.

Yeah, that's weird, I don't get that failure locally either. Just fix it though.

sindresorhus · 2016-11-22T18:45:27Z

test/fixture/typescript/simple.ts

@@ -0,0 +1,6 @@
+import { test } from 'ava'


import {test} from 'ava';

And use semi-colons.

sindresorhus · 2016-11-22T18:46:17Z

package.json

@@ -89,7 +89,7 @@
    "arr-flatten": "^1.0.1",
    "array-union": "^1.0.1",
    "array-uniq": "^1.0.2",
-    "arrify": "^1.0.0",
+    "arrify": "^1.0.1",


Don't do unrelated changes.

sindresorhus · 2016-11-22T18:46:21Z

package.json

@@ -154,7 +154,7 @@
    "resolve-cwd": "^1.0.0",
    "semver": "^5.3.0",
    "set-immediate-shim": "^1.0.1",
-    "source-map-support": "^0.4.0",
+    "source-map-support": "^0.4.5",


I just run an npm install arrify source-map-support --save by mistake

Restored old version.

sindresorhus · 2016-11-22T18:49:01Z

lib/ext/ts.js

@@ -0,0 +1,116 @@
+'use strict';
+/**
+ * AVA File Extention for typescript


Extention => Extension

typescript => TypeScript

sindresorhus · 2016-11-22T18:52:14Z

lib/test-worker.js

+				extTs.register();
+				break;
+			default:
+				// not supported ext?


It should throw an useful error here.

done.

throw new Error('`--extension ' + ext + '` is not supported by AVA.')

sindresorhus · 2016-11-22T18:53:18Z

lib/watcher.js

 			runOnlyExclusive: runOnlyExclusive
-		}).then(function (runStatus) {
+		};
+		if (extensions) {


This is an internal API. We can just make passing the extensions argument required and drop this if-statement.

sindresorhus · 2016-11-22T18:54:16Z

lib/cli.js

@@ -100,6 +103,11 @@ 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.');
 	}

+	if (cli.flags.extension) {
+		cli.flags.extension = arrify(cli.flags.extension);
+		cli.flags.extension.push('js');


As we discussed in avajs/ava-files#9 (comment), the option should override js.

I changed to this:

if (cli.flags.extension) { cli.flags.extension = arrify(cli.flags.extension); } else { cli.flags.extension = ['js']; }

Couldn't you just do this?

cli.flags.extension = arrify(cli.flags.extension || 'js');

@jedmao great idea, thanks!

sindresorhus · 2016-11-22T18:55:50Z

lib/cli.js

@@ -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    Set test file extention. `ts` is for Typescript',


Test file extension, for example `ts` for TypeScript (Can be repeated)

sindresorhus · 2016-11-22T18:56:32Z

lib/caching-precompiler.js

+			// here for other exts in the future
+
+			default:
+				// not supported(yet)


It should throw an useful error here.

done.

throws the same error as the above.

sindresorhus · 2016-11-23T05:57:54Z

I don't think you pushed your changes.

Merge conflict also needs fixing.

huan · 2016-11-23T06:36:15Z

@sindresorhus Yes, I'm still fighting with the unit tests.

Because of the new extensions option parameter, some unit tests broken.

Will push after I fixed them.

…#1109

huan · 2016-11-23T09:35:30Z

Just finished all the unit tests with the latest code in this PR branch.

ava-files has updated PR to latest version as well.

Please let me know if I missed anything. :)

sindresorhus · 2016-11-29T06:34:36Z

lib/ext/ts.js

+	};
+}
+
+var extTs = {


Just export this directly.

sindresorhus · 2016-11-29T06:35:44Z

lib/ext/ts.js

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


Doesn't ts-node already do this?

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?

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.

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

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

sindresorhus · 2016-11-29T06:36:07Z

lib/ext/ts.js

+ *
+ * Ts-Node version
+ *
+ */


Remove comment.

novemberborn

@zixia very cool!

There's some merge conflicts now, probably because we changed the code to use more ES2015 features. Sorry about that, though on the plus side you get to use ES2015!

novemberborn · 2016-12-04T14:57:57Z

lib/caching-precompiler.js

@@ -47,8 +48,29 @@ 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(/^\./, '');


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

novemberborn · 2016-12-04T15:02:19Z

lib/caching-precompiler.js

+	var fileExt = path.extname(filePath).replace(/^\./, '');
+	var isCustomExt = this.extensions.filter(function (ext) {
+		return (ext !== 'js' && ext === fileExt);
+	}).length > 0;


This could be simplified to:

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

novemberborn · 2016-12-04T15:05:12Z

lib/caching-precompiler.js

+		}
+	}
+
+	if (!result) {


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

novemberborn · 2016-12-04T15:07:03Z

lib/ext/ts.js

+	typescript = require('typescript');
+} catch (err) {
+	console.error('AVA: `--extensions=ts` require TypeScript to be installed.');
+	throw err;


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.

novemberborn · 2016-12-04T15:12:38Z

lib/ext/ts.js

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


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.

huan · 2016-12-04T18:01:37Z

@novemberborn I love ES2015 too. Had just followed your suggestions and resolved all the conflicts.

novemberborn · 2016-12-05T17:22:41Z

lib/caching-precompiler.js

+	var result;
+
+	if (isCustomExt) {
+		switch (fileExt) {


@zixia could you change this to an if statement?

novemberborn · 2016-12-05T17:25:58Z

lib/caching-precompiler.js

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


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

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?

novemberborn · 2016-12-05T17:27:36Z

lib/ext/ts.js

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


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

novemberborn · 2016-12-05T17:27:51Z

lib/ext/ts.js

+	};
+}
+
+module.exports = module.exports.default = {


Just use exports.transpile = typescriptTranspile.

I think we need to keep register() with tranpile(), as I described here: #1122 (comment)

If there has any alternative way to do this, please let me know, thanks.

novemberborn · 2016-12-05T17:29:39Z

lib/test-worker.js

+				throw new Error('AVA: `--extension=' + ext + '` is not supported by AVA.');
+		}
+	});
+}


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.

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:

There are almost no TypeScript dependencies in NPM(node_modules)

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.

huan · 2016-12-06T06:32:35Z

Thank you guys for all the replies of this PR.

Please let me know if I missed any important replies because this conversation is growing fast. ;-p

novemberborn · 2016-12-06T12:15:44Z

@zixia could you explain why you think register is needed in the workers? The CachingPrecompiler already compiles the test files, and the workers load the resulting code. On-the-fly compilation would only apply to files required by the test file. I'm arguing that, though not ideal, it's better to be consistent with our Babel implementation and not automatically transpile such dependencies. Thus, register would not be needed.

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?

There are none, currently, since .js is hardcoded in a few places. I'm suggesting you check for .ts after the hasFlag tests, before API is instantiated. Around here.

Zooming out a bit, CachingPrecompiler was really written for Babel. It uses the Babel config to salt the cache and it automatically loads Babel when it needs to compile something. Your proposed changes still cause the Babel code paths to execute and don't allow the TypeScript compiler to be configured. I think that's OK at this stage but we'll have to solve those issues at a later stage. @avajs/core thoughts?

@jedmao perhaps we should revisit .tsx once this has landed? There's a parallel with .jsx which currently doesn't work either. Maybe we can treat these as aliases and just assume the existing .js / .ts setup supports JSX. It's complicated though because JSX doesn't necessarily mean React.

huan · 2016-12-08T03:18:22Z

@novemberborn got it.

The reason I think register is needed in the workers is because if I do not do this, my test will not work anymore.

I believe that's because my tests have to import pure TypeScript module file: there is all kind of TypeScript files in my source tree, and I never compile them, just import them in my tests.

Will check them and reply you with the details later.

novemberborn · 2016-12-09T17:10:39Z

@zixia:

The reason I think register is needed in the workers is because if I do not do this, my test will not work anymore.

I believe that's because my tests have to import pure TypeScript module file: there is all kind of TypeScript files in my source tree, and I never compile them, just import them in my tests.

Will check them and reply you with the details later.

Right, that's the same issue we have with our Babel support: source files are not automatically transpiled so users need to add babel-core/register to AVA's require configuration.

This is by no means ideal, but automatically including babel-core/register has a measurable impact on performance for those users who do not need it. I imagine the same applies to TypeScript.

I've been playing with performance optimizations in this area (e.g. #1082). Really I'd like to have users opt-in to a particular behavior for their tests and source files, and then have everything be automatic.

For now though I'm suggesting we do not automatically load TypeScript when running tests. The behavior would be the same as for users relying on babel-core/register.

This PR already introduces many subtle changes to AVA's behavior that should be factored out. That's a larger task and I think this PR provides enough value as it stands today, so we should try and land it. But not with automatic TypeScript loading.

huan · 2016-12-10T06:37:22Z

Hi @novemberborn ,

Got your idea, and thanks for the explanations.

There are none, currently, since .js is hardcoded in a few places. I'm suggesting you check for .ts after the hasFlag tests, before API is instantiated. Around here.

done.

Right, that's the same issue we have with our Babel support: source files are not automatically transpiled so users need to add babel-core/register to AVA's require

done.

I had removed extTs.register() part from lib/test-worker.js, and add the following json part to package.json, TypeScript work without any problem again.

  "ava": {
    "require": [
      "ts-node/register"
    ]
  },

For more about TypeScript register, here's some discussions about it: microsoft/TypeScript#1823

Have a good weekend!

huan · 2016-12-10T07:14:10Z

I found a strange unit test failure in test/cli.js at here: https://github.com/zixia/ava/blob/d7bfa76fb708b6d918f1c821df0417d79e96cde4/test/cli.js#L383

We run cli.js in cwd, however, we pass resolveTestsFrom to AvaFiles at here, which is /tmp/ instead of /tmp/xxxx/, as the result, test fail.

I have no idea of it because I did not touch this part this week and everything is ok before. Will check it later.

asvetliakov · 2016-12-16T03:27:26Z

Any news about PR ? I'd like to adopt ava in my project, but separate TS precompilation (and as i understand, the ava now just doesn't work with file extensions rather than .js) is huge stopper for me.

huan · 2016-12-16T03:46:25Z

@asvetliakov Before this PR be merged, you can try this new feature by clone the source with npm link:

$ git clone https://github.com/zixia/ava-files.git
$ cd ava-files
$ npm install && npm link
$ cd ..

$ git clone https://github.com/zixia/ava.git
$ cd ava
$ git checkout typescript
$ npm install & npm link ava-files && npm link

Then you can use ava with TypeScripe in your project like:

$ cd my-project
$ npm link ava
$ ./node_modules/.bin/ava --extension=ts

Please let me know if there's any issue with your project if you have time to try it.

BTW: I love your Huject.

Good luck! :)

novemberborn · 2016-12-19T15:50:34Z

Hi @zixia, I have some concerns with the direction we're taking here.

As already mentioned it's not clear how this would work for source files without running into the same performance issues we already see with Babel. The TypeScript test files won't get enhanced assertions or protection against improper t.throws() usage either. It's hard to communicate this to users when the way TypeScript support is enabled is to specify an extensions option.

I've been doing a lot of thinking on how we can better solve these problems. Not only for TypeScript but for Babel-based projects as well. I've started writing an RFC with my specific proposal but I'm not ready to share it yet. I'm currently on holiday, and the weather is awfully nice (except when there's ⛈ like right now), so I can't commit to any particular publication date.

I didn't want to leave you hanging though. As I see it right now I don't think we can land this pull request. I'm really sorry about that, since you've put in a lot of work already.

I hope you'll give my proposal a chance. It should enable us to make AVA's TypeScript support really awesome.

Thanks.

huan · 2016-12-19T16:02:38Z

@novemberborn Understand, and please let me know when you made new progress on your RFC. :)

Have a nice holiday!

novemberborn · 2016-12-20T14:11:23Z

@zixia see #1159.

novemberborn · 2017-01-17T17:11:16Z

We'll be taking a different direction with regards to TypeScript support, see RFC001. @zixia I'll let you know when the Babel side of that has stabilized sufficiently for you (or somebody else) to take on the TypeScript implementation.

huan · 2017-01-17T17:24:47Z

@novemberborn No problem, looking forward hearing from you.

huan added 5 commits November 15, 2016 15:25

Support to Run TypeScript test file(.ts) directly without Precompiling …

30bf714

…#1109

make xo happy

0a0e448

follow feedbacks from @sindresorhus #1109

c3bf9b3

add unit test for --extensions ts #1109

7ea3468

not require TypeScript when no --extension set #1109

55ea370

sindresorhus force-pushed the master branch 2 times, most recently from 8610723 to 4acb78f Compare November 22, 2016 17:46

sindresorhus requested changes Nov 22, 2016

View reviewed changes

sindresorhus changed the title ~~Typescript without compiling~~ Add support for TypeScript without precompiling Nov 22, 2016

huan added 2 commits November 23, 2016 05:11

follow requested changes #1122 #1109

2708e40

handle js in worker #1122

4024791

huan added 14 commits November 23, 2016 07:18

rename compile to transpile to follow existing names

a925e0e

fix watcher unit tests

c0b276a

Support to Run TypeScript test file(.ts) directly without Precompiling …

47b7dfc

…#1109

make xo happy

91386f6

follow feedbacks from @sindresorhus #1109

d7bfa76

add unit test for --extensions ts #1109

c76199c

not require TypeScript when no --extension set #1109

c9c1020

follow requested changes #1122 #1109

6ad050a

handle js in worker #1122

5629601

rename compile to transpile to follow existing names

77f456c

fix watcher unit tests

e382848

only require extTs when necessary #1122

5eb9ba1

merge

685a4fe

code clean #1122

5441111

huan added 2 commits November 26, 2016 03:59

https://github.com/avajs/ava/pull/1122#discussion_r89566725

58690ad

use [].concat() instead of arrify

d91ed25

sindresorhus reviewed Nov 29, 2016

View reviewed changes

follow @sindresorhus

accbddb

novemberborn requested changes Dec 4, 2016

View reviewed changes

huan added 3 commits December 4, 2016 17:31

follow @novemberborn

ef26c97

merge

aac2fc1

fix tab/space

2ea3db7

novemberborn requested changes Dec 5, 2016

View reviewed changes

huan added 2 commits December 6, 2016 07:07

follow @novemberborn

8f2bca0

get rid of ts-node dev-dependence

41043e7

follow @novemberborn

6d071e2

novemberborn mentioned this pull request Dec 20, 2016

RFC 001: Improving language support #1159

Merged

upgrade typescript from 2.0 to 2.1

d577ff5

novemberborn closed this Jan 17, 2017

jednano mentioned this pull request Feb 7, 2017

Meta avajs/atom-ava#2

Open

Add support for TypeScript without precompiling #1122

Add support for TypeScript without precompiling #1122

Conversation

huan commented Nov 21, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus Nov 23, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sindresorhus commented Nov 23, 2016

huan commented Nov 23, 2016 • edited Loading

huan commented Nov 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huan commented Dec 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huan commented Dec 6, 2016

novemberborn commented Dec 6, 2016

huan commented Dec 8, 2016

novemberborn commented Dec 9, 2016

huan commented Dec 10, 2016

huan commented Dec 10, 2016

asvetliakov commented Dec 16, 2016

huan commented Dec 16, 2016 • edited Loading

novemberborn commented Dec 19, 2016

huan commented Dec 19, 2016 • edited Loading

novemberborn commented Dec 20, 2016

novemberborn commented Jan 17, 2017

huan commented Jan 17, 2017

huan commented Nov 21, 2016 •

edited

Loading

sindresorhus Nov 23, 2016 •

edited

Loading

huan commented Nov 23, 2016 •

edited

Loading

huan commented Dec 16, 2016 •

edited

Loading

huan commented Dec 19, 2016 •

edited

Loading