Added support for babel config under package.json/ava key (Fixes #448) #573

spudly · 2016-02-25T20:30:47Z

Here's an initial commit that allows package owners to specify a babel config for tests under the package.json/ava key.

This conforms to the proposal in the following comment: #448 (comment)

I chose not to implement an extends key discussed in the thread because 1) it adds to the complexity, 2) I personally don't need it, and 3) how to solve that request is still somewhat unresolved. If needed, someone can add that later in another pull request.

Let me know what you think. I'm a little unsure about whether my method for obtaining babelConfig is the best one.

mention-bot · 2016-02-25T20:30:56Z

By analyzing the blame information on this pull request, we identified @novemberborn, @sotojuan and @ariporad to be potential reviewers

sindresorhus · 2016-02-26T08:15:51Z

cli.js

@@ -97,7 +97,8 @@ var api = new Api(cli.input.length ? cli.input : arrify(conf.files), {
 	failFast: cli.flags.failFast,
 	serial: cli.flags.serial,
 	require: arrify(cli.flags.require),
-	cacheEnabled: cli.flags.cache !== false
+	cacheEnabled: cli.flags.cache !== false,
+  babelConfig: conf.babel,


Wrong indentation here.

sindresorhus · 2016-02-26T08:19:15Z

Good start :) This will need tests and docs too.

spudly · 2016-02-26T15:39:33Z

Fixed indentation, added tests, and added docs.

kentcdodds · 2016-02-27T05:57:18Z

readme.md

+}
+```
+
+If you do not specify a "babel" key in your ava configuration, or if you set it to `"default"`, AVA will transpile the test files with AVA's default babel configuration.


Is this default configuration defined anywhere else in the docs? If not, it should be. Then this should link to that.

👍 I don't believe it is defined anywhere, but it should be.

vadimdemedes · 2016-02-28T10:29:25Z

Hey @spudly, could you rebase from master to resolve a conflict? Thanks for the great work!

spudly · 2016-02-29T12:24:17Z

@vdemedes

It's been rebased.

vadimdemedes · 2016-02-29T15:01:36Z

Great, I think it's good to go!

@sindresorhus @novemberborn @jamestalmage ?

novemberborn · 2016-02-29T18:09:07Z

lib/caching-precompiler.js

 		};
+
+		if (!babelConfig || babelConfig === 'default') {
+			return objectAssign({}, baseOptions, {


No need for the empty object here, baseOptions is already fresh so it's safe to mix other properties into it.

novemberborn · 2016-02-29T18:16:43Z

Haven't looked at the tests yet, sorry. Been staring at AVA issues all day :)

Could you follow up on #448 (comment)?

Looking really good though, nice work.

jamestalmage · 2016-02-29T22:07:07Z

lib/caching-precompiler.js

+		}
+
+		if (babelConfig === 'inherit') {
+			return objectAssign({}, baseOptions, {babelrc: true});


As discussed, we will want to ensure powerAssert is added as a plugin regardless.

novemberborn · 2016-03-01T12:23:26Z

The tests exercise caching-compiler, which is good. I'd like to see some integration tests in test/cli, though that does not have to hold up this PR. Happy to undertake that myself or help you @spudly if you're interested. It might be a bit tricky to verify the differing Babel behaviors.

Right now package.json/ava/babel allows any Babel option to be changed. To avoid footguns though I think filename, sourceMaps, ast and inputSourceMap should not be configurable.

@spudly thanks for the hard work on this. It's solving so many of our issues!

spudly · 2016-03-01T15:09:13Z

Pushed a new commit & rebased. I believe I've addressed all of the concerns that were discussed, with the exception of the babel: false option, which I believe should be addressed in a separate pull request.

spudly · 2016-03-01T15:16:28Z

Can anyone help explain why these 44 tests are failing?

https://travis-ci.org/sindresorhus/ava/jobs/112878667

They're unrelated to my changes - the master branch of AVA fails the same tests. These tests were passing yesterday, but today they fail. Perhaps one of the dependencies had a release that broke with semver?

wyze · 2016-03-01T15:20:54Z

Might be related to babel 6.6 release yesterday.

I noticed that your previous build used 6.5.2 while this one used 6.6.

novemberborn · 2016-03-01T15:21:35Z

test/caching-precompiler.js

 test('creation with new', function (t) {
 	var tempDir = uniqueTempDir();
-	var precompiler = new CachingPrecompiler(tempDir);
+	var precompiler = new CachingPrecompiler(tempDir, null);


Should pass 'default' here (and below).

MoOx · 2016-03-01T15:34:36Z

Failures may be similar to MoOx/phenomic#229 (or may not)

novemberborn · 2016-03-01T15:38:43Z

@spudly did a fresh checkout and install, master still passes but this branch does not.

novemberborn · 2016-03-01T15:42:49Z

@spudly ah! The failing tests use api.js directly, without setting the babelConfig option. Now I feel bad 😢

I still think using the default config in cli.js is the right move, but maybe add the !babelConfig || babelConfig === 'default' clause back in caching-precompiler.js. Quite bothersome passing it in the 44 tests that instantiate Api.

spudly · 2016-03-02T13:55:15Z

My bad, I must have made a mistake when I checked out master and ran the tests again yesterday. Did it again this morning and the tests are passing. I'll fix this very soon.

spudly · 2016-03-02T16:40:34Z

I'm stumped guys. If I do the following, all tests pass as expected.

git clone https://github.com/sindresorhus/ava.git
cd ava
git reset --hard 58e1c561296a409a9557ceb1c7684685404c4278
npm install
npm test

But if I do this, 44 tests fail:

git clone https://github.com/spudly/ava.git
cd ava
git reset --hard 58e1c561296a409a9557ceb1c7684685404c4278
npm install
npm test

Both repos are being reset to the same initial commit (the one before any of my changes), so there should be absolutely no difference between the two, right?

kentcdodds · 2016-03-02T16:41:56Z

I'm running both of those series of commands now. I'll let you know what I get

kentcdodds · 2016-03-02T16:45:46Z

Everything passed for me for both of those. Sounds like an environment issue...

…f babel plugins

spudly · 2016-03-07T12:56:42Z

Is there a reason we have return statements on cli.js:13 and cli.js:96? It's causing lint errors with the latest XO config.

spudly · 2016-03-07T13:07:38Z

Nevermind. running npm install fixed that issue. Still curious though, why are those return statements there?

spudly · 2016-03-07T13:11:39Z

made requested changes and rebased again.

novemberborn · 2016-03-07T14:55:50Z

Is there a reason we have return statements on cli.js:13 and cli.js:96?

They're shorthand for exiting the CLI.

novemberborn · 2016-03-07T15:02:36Z

@spudly what's the reason for adding the babel-cli dependency in package.json?

spudly · 2016-03-07T15:26:51Z

@novemberborn,

That dependency was added by @jamestalmage in the babel-plugin-for-integration-tests branch, which is merged into my code.

vadimdemedes · 2016-03-07T15:31:02Z

I guess "outside" commits shouldn't be in this PR, should they?

novemberborn · 2016-03-07T15:36:19Z

That dependency was added by @jamestalmage in the babel-plugin-for-integration-tests branch, which is merged into my code.

Ah!

@vdemedes are you in Gitter at the moment? Let's see about landing the integration test plugin in master etc.

Fixes #448

novemberborn · 2016-03-07T15:54:55Z

Landed in a03f826 yay! (Messed up the commit so it didn't close automatically… oops)

novemberborn · 2016-03-07T15:57:26Z

Awesome work, thanks @spudly!

spudly · 2016-03-07T16:00:58Z

Nice! Thanks for the help everyone!

sindresorhus · 2016-03-07T16:04:32Z

Yay! Nice work @spudly 💃

kentcdodds · 2016-03-07T16:06:22Z

Hooray! 🎉🎉🎉

Wildhoney · 2016-03-08T12:48:05Z

👏 👏

For those wondering, it appears as though this hasn't yet been published to npm — however a npm i https://github.com/sindresorhus/ava will suffice for the moment.

vadimdemedes · 2016-03-08T12:50:58Z

tleunen · 2016-03-09T02:59:09Z

Can we get a cli option for that as well? I prefer specifying it there instead of adding a config in the package.json

jamestalmage · 2016-03-09T03:20:27Z

@tleunen

Please raise an issue.

sindresorhus reviewed Feb 26, 2016
View reviewed changes

sindresorhus mentioned this pull request Feb 27, 2016

Written a React recipe (with an example) #462

Closed

kentcdodds reviewed Feb 27, 2016
View reviewed changes

novemberborn reviewed Feb 29, 2016
View reviewed changes

jamestalmage reviewed Feb 29, 2016
View reviewed changes

novemberborn reviewed Mar 1, 2016
View reviewed changes

sindresorhus force-pushed the master branch from c6214aa to 2f10448 Compare March 2, 2016 07:42

Stephen Sorensen added 8 commits March 7, 2016 07:50

added tests for customizable babel config

bc49364

added documentation for customizable babel config

0044f9a

fixed test for node 0.12 compatibility

9a62bed

enforced constraints on babel config

d833f52

handle undefined babelConfig to fix api unit tests

eb6d024

better tests for appending powerAssert and transformRuntime to list o…

8517681

…f babel plugins

added integration tests for custom babel config

c052757

added babelConfig to compiler cache salt

03850ad

return promise in api test

f167ea3

novemberborn pushed a commit that referenced this pull request Mar 7, 2016

Close PR #573: Added support for babel config under package.json/ava key

a03f826

Fixes #448

novemberborn closed this Mar 7, 2016

tleunen mentioned this pull request Mar 11, 2016

[CLI] No option for the babel config #637

Closed

jamestalmage mentioned this pull request Mar 22, 2016

Babel plugins not loading when overridden in package.json AVA section #674

Closed

Added support for babel config under package.json/ava key (Fixes #448) #573

Added support for babel config under package.json/ava key (Fixes #448) #573

Conversation

spudly commented Feb 25, 2016

mention-bot commented Feb 25, 2016

sindresorhus Feb 26, 2016

Choose a reason for hiding this comment

sindresorhus commented Feb 26, 2016

spudly commented Feb 26, 2016

kentcdodds Feb 27, 2016

Choose a reason for hiding this comment

jamestalmage Feb 29, 2016

Choose a reason for hiding this comment

vadimdemedes commented Feb 28, 2016

spudly commented Feb 29, 2016

vadimdemedes commented Feb 29, 2016

novemberborn Feb 29, 2016

Choose a reason for hiding this comment

novemberborn commented Feb 29, 2016

jamestalmage Feb 29, 2016

Choose a reason for hiding this comment

novemberborn commented Mar 1, 2016

spudly commented Mar 1, 2016

spudly commented Mar 1, 2016

wyze commented Mar 1, 2016

novemberborn Mar 1, 2016

Choose a reason for hiding this comment

MoOx commented Mar 1, 2016

novemberborn commented Mar 1, 2016

novemberborn commented Mar 1, 2016

spudly commented Mar 2, 2016

spudly commented Mar 2, 2016

kentcdodds commented Mar 2, 2016

kentcdodds commented Mar 2, 2016

spudly commented Mar 7, 2016

spudly commented Mar 7, 2016

spudly commented Mar 7, 2016

novemberborn commented Mar 7, 2016

novemberborn commented Mar 7, 2016

spudly commented Mar 7, 2016

vadimdemedes commented Mar 7, 2016

novemberborn commented Mar 7, 2016

novemberborn commented Mar 7, 2016

novemberborn commented Mar 7, 2016

spudly commented Mar 7, 2016

sindresorhus commented Mar 7, 2016

kentcdodds commented Mar 7, 2016

Wildhoney commented Mar 8, 2016

vadimdemedes commented Mar 8, 2016

tleunen commented Mar 9, 2016

jamestalmage commented Mar 9, 2016