Adds support for ava.config.js files #1761

good-idea · 2018-04-02T22:59:20Z

Fixes #520

pvdlg · 2018-04-03T17:57:59Z

Have you checked cosmiconfig? It would fulfill a lot of the requirements, and the one missing might be implement over there.

novemberborn · 2018-04-03T18:58:08Z

We'd end up using very little of cosmiconfig. It's relatively straight-forward to hook this up directly in AVA.

@good-idea thanks for your PR. I've been quite busy lately so I make need another few days before I can get back to you.

good-idea · 2018-04-03T19:25:34Z

@novemberborn No hurry, I don't think I'll be able to chip away at this until the end of the week.

This is my first contribution to OSS 🤓 so any feedback is welcome!

novemberborn · 2018-04-04T08:19:39Z

Some quick responses to your questions (I'll try and get to the other stuff later):

This is still using pkg-conf under the hood. But, we could easily remove it as a dependency, then use load-json-file instead. Any reasons for/against?

No strong feelings either way, though I suppose I should look into what pkg-conf does exactly, and I haven't done that yet. When in doubt, lean toward making fewer changes 😉

I've made some dirs in test/fixture/load-config that I'll use in writing more tests. But I'm not sure how to test cases when there is no package.json, because the test will travel up to the root ava dir and see its package.json. (The current behavior is to keep searching up until /, then finally return false if nothing is found)

No that's fine. The test case we're looking for is where there are package.json files, both with and without an "ava" configuration.

Some tests are failing, but they don't seem to be related

A newer Babel 7 is being picked up in one of the test jobs, and it's causing test failures. I'm a little surprised at that, to be honest! Feel free to ignore the Travis run with FRESH_DEPS=true, and AppVeyor as well since it doesn't run any other tests after the fresh-deps one fails.

novemberborn

Great start @good-idea! There seems to be some confusion around where the ava.config.js file is supposed to be. It's either next to a package.json file or in the current directory. I don't want it to be a secondary way for AVA to determine the project directory. If this proves too confusing we can change it, but for now I like the simplicity of AVA determining what is a "project" based on package.json files only.

novemberborn · 2018-04-07T16:49:38Z

lib/load-config.js

+const findConfigFiles = () => {
+	const packageFile = findPackageDotJson();
+	const configFile = findConfigFile(
+		packageFile ? path.dirname(packageFile) : process.cwd(),


Let's simplify this. We can keep the current pkg-conf logic to find the package.json, if any. Afterwards, the ava.config.js file must be in the projectDir. We won't walk the file system to find it. (This means the findFileUp logic above is not necessary.)

novemberborn · 2018-04-07T16:54:10Z

test/cli.js

@@ -40,7 +40,9 @@ function execCli(args, opts, cb) {

 		child.on('close', (code, signal) => {
 			if (code) {
-				const err = new Error(`test-worker exited with a non-zero exit code: ${code}`);
+				const err = new Error(
+					`test-worker exited with a non-zero exit code: ${code}`,


Looks like you're using Prettier. Could you exclude unrelated changes from the PR?

novemberborn · 2018-04-08T16:04:59Z

@good-idea if you could merge in (or rebase on) master, that should fix some of the CI issues.

good-idea · 2018-04-08T16:48:51Z

@novemberborn thanks, I'll working on this later today!

And adds test for factory function in config file

Should have merged before but I rebased?

thought i fixed this!

good-idea · 2018-04-10T00:30:11Z

@novemberborn I think this just about does it, let me know if you see anything else that should change!

novemberborn

@good-idea this is looking great!

I haven't actually played with the implementation yet, and I must admit I haven't really looked at the tests just yet either. There's a few things that should be addressed (see inline comments).

Looking at the diff on GitHub it seems to want to add things that are already in master. Not quite sure what's happening. Perhaps try merging in master or rebasing 😄

I'm very excited about landing this!

novemberborn · 2018-04-15T16:39:55Z

lib/load-config.js

+'use strict';
+const path = require('path');
+const fs = require('fs');
+// ESM seems to break `path` and `fs`, require them first before setting it up..


esm shouldn't impact require() calls though. Happy to see the Node.js builtins loaded first, but your comment hints at an esm bug. Is this still happening for you?

I haven't looked into why, but if I set up ESM first like so:

'use strict'; require = require('esm')(module); const path = require('path'); console.log(path);

path is null.

This works for me, Node.js 8.11.0, and esm@3.0.22.

esm@3.0.15 was installed, I updated to 3.0.22 and it now works as expected.
I also moved esm from devDependencies to dependencies, since it's now being used in the main code -- does this make sense?

novemberborn · 2018-04-15T16:40:04Z

lib/load-config.js

+
+const loadConfigFile = projectDir => {
+	const loaded = requireESM(path.resolve(projectDir, 'ava.config.js'));
+	const config = (loaded.__esModule) ? loaded.default : loaded;


Does esm take care of this?

no, it doesn't seem to. There's a test for this that fails if you don't do the check for __esModule.

novemberborn · 2018-04-15T16:40:44Z

lib/load-config.js

+	if (config.then) {
+		throw new Error('Config file must not return a promise');
+	}
+	return (typeof config === 'function') ? config(projectDir) : config;


Let's pass {projectDir}, so we can extend this with other options later.

Ah, I meant doing config({projectDir}), not loadConfigFile({projectDir}).

novemberborn · 2018-04-15T16:42:44Z

lib/load-config.js

+		projectDir
+	};
+
+	return Object.assign(config, defaults, {_meta});


This copies the defaults into config. The _meta bit seems unnecessary to me.

Better would be Object.assign({}, defaults, config, {projectDir}).

novemberborn · 2018-04-15T16:43:41Z

profile.js

@@ -38,9 +38,9 @@ const conf = pkgConf.sync('ava', {
 	}
 });

-const filepath = pkgConf.filepath(conf);
-const projectDir = filepath === null ? process.cwd() : path.dirname(filepath);
+console.log(conf);


Don't forget to remove 😉

novemberborn · 2018-04-15T16:57:49Z

Oh, one more thing. The fileType values in

ava/lib/babel-config.js

Line 136 in 718349e

fileType: 'JSON',

need to change to 'JS' when the config originates from a .js file. I'm happy to fix and test that myself though.

good-idea · 2018-04-17T19:08:12Z

@novemberborn This last update should address the notes in your review -- I didn't look into the fileType issue in babel-conifg yet, though. I updated the initial PR description to include a todo for this.

novemberborn · 2018-04-18T08:21:41Z

@good-idea fantastic. I probably won't be able to look at this until the weekend, but thank you for your hard work on this 👍

sindresorhus · 2018-04-23T15:27:44Z

lib/load-config.js

@@ -0,0 +1,33 @@
+'use strict';
+const requireEsm = require('esm')(module); // eslint-disable-line no-global-assign


I think think // eslint-disable-line no-global-assign is needed.

novemberborn

I've left some comments below.

I'll have a look at the fileType: 'JS thing.

I also want to see if we can restrict this to supporting just export default / ESM.

novemberborn · 2018-04-29T16:24:00Z

lib/load-config.js

+
+const loadConfigFile = projectDir => {
+	const loaded = requireEsm(path.resolve(projectDir, 'ava.config.js'));
+	const config = (loaded.__esModule) ? loaded.default : loaded;


I want to play around with the esm options. I think we can make it always treat these files as ESM, so we don't have to support the module.exports workarounds etc. Would be neat to start with that. What do you think @sindresorhus?

novemberborn · 2018-04-29T16:26:59Z

lib/load-config.js

+	if (config.then) {
+		throw new Error('Config file must not return a promise');
+	}
+	return (typeof config === 'function') ? config({projectDir}) : config;


The config({projectDir}) return value also must not be a promise. Happy to fix this up myself.

novemberborn · 2018-04-29T16:28:28Z

readme.md

+};
+```
+
+The config file may also export a factory function, which will be called with an object containing a `projectDir` property. This function must return a config object, and cannot be a promise.


Perhaps:

This function must return a config object, it mustn't return a promise.

novemberborn · 2018-04-29T16:31:07Z

readme.md

+
+```js
+const config = ({projectDir}) => {
+	if (projectDir === '...') {


projectDir will be an absolute path, so it'd be good if the example didn't show dots which imply it's somehow relative.

Providing the directory is useful if you have share AVA config amongst projects, e.g. in a work setting or a monorepo. You could then differentiate between projects when necessary. Perhaps this is something we should cover in a recipe, outside of this PR.

novemberborn · 2018-04-29T16:32:25Z

readme.md

+export default config;
+```
+
+Or, return different configuration based on the environment: `NODE_ENV=development ava`


I'm not sure about advocating AVA to be run with different NODE_ENV settings. It'd be more interesting to detect CI environments — although at that point we should forward the detection to the function. Perhaps remove for now and track in a follow-up issue?

good-idea · 2018-04-30T18:50:32Z

@novemberborn I made some changes to address the above -- there another big diff with package-lock.json, so I did the same as before. Let me know if everything is looking OK.

novemberborn · 2018-05-07T16:08:14Z

@good-idea I haven't had a chance yet to look into this. I just realized we need some changes with Babel (#1789) so I might try and land that before doing my work here.

Anyway this is super close so once that unblocks we should be good to go.

novemberborn · 2018-05-20T16:42:17Z

@good-idea just wanted to say I haven't forgotten about this 😄 #1789 is pretty much done so I'm hoping to land that next weekend. I'll then try and get this in as well before cutting a release.

good-idea · 2018-05-20T19:49:19Z

@novemberborn No hurry on my end! I've been using my own fork for now :)
Looking forward to the next release

novemberborn · 2018-05-28T16:41:49Z

@good-idea I've merged latest master and pushed documentation improvements and some cleanup of the config loading. We're also now using just ESM.

There's a few scenarios that are lacking tests. Once I've done that I'll land this 🎉

good-idea added 2 commits April 2, 2018 15:17

Replaces pkg-conf with lib/load-config [WIP]

e9e688f

Changes handling of defaults, implements in profile.js

6343a1d

novemberborn requested changes Apr 7, 2018

View reviewed changes

good-idea added 11 commits April 8, 2018 10:05

Replaces pkg-conf with lib/load-config [WIP]

e98b150

Changes handling of defaults, implements in profile.js

c443e54

Un-pretties test/cli.js

0755a1b

Simplifies config loading

27038fc

Adds support for ESM import

64ef0da

And adds test for factory function in config file

Fixes lint errors

fab1349

Merge from remote, clear up conflicts

27444e6

Should have merged before but I rebased?

Lints test fixtures

5e90143

Changes implementation of getting the project dir from the config

ceb8d85

Fixes prettied lines in test/cli.js

2f2af6e

thought i fixed this!

Removes object spread to make node 6-safe

c5d9039

novemberborn requested changes Apr 15, 2018

View reviewed changes

good-idea added 2 commits April 17, 2018 11:38

Merge branch 'master' into config-file

e5be1ae

Removes '_meta' from config in favor of '{projectDir}'

2ef9a6a

good-idea added 3 commits April 22, 2018 11:24

Changes factory function argument to {projectDir}

2cbfd87

Updates esm and moves to dependencies

e3ae8c4

Merges in changes to master, fixes conflicts

4079be1

sindresorhus added 2 commits April 23, 2018 22:26

Update ava.config.js

7f24c2b

Update package.json

6ad5a42

sindresorhus reviewed Apr 23, 2018

View reviewed changes

sindresorhus and others added 3 commits April 23, 2018 22:31

Update readme.md

fed12af

Update load-config.js

6b8ba43

Removes eslint-disable comment

46f724f

novemberborn reviewed Apr 29, 2018

View reviewed changes

good-idea added 4 commits April 30, 2018 11:06

Update readme.md

7886b2b

Disallows config to be or return promises

13a333a

Merge in latest upstream master

0f87d09

Cleans up package-lock.json diff

d9b091d

novemberborn changed the title ~~[WIP] Adds support for ava.config.js files~~ Adds support for ava.config.js files May 20, 2018

novemberborn added 5 commits May 28, 2018 16:41

Merge branch 'master' into pr/1761

4215a47

Fix package-lock, keep esm version the same as master for now

1cff8f1

Mention ava.config.js in recipes

02ca5c1

Update README

cece1e7

Refactor loading, only use ESM

cfc9d70

novemberborn approved these changes May 28, 2018

View reviewed changes

novemberborn mentioned this pull request May 28, 2018

Restore Babel cache invalidation #1817

Closed

novemberborn added 4 commits May 30, 2018 14:40

Merge branch 'master' into pr/1761

79523ff

Clean up fixtures

3b5536c

Test more edge cases

2bb057b

Exit CLI with nicely formatted error when loading ava.config.js fails

43e06dd

novemberborn merged commit 16f4742 into avajs:master May 30, 2018

billyjanitsch mentioned this pull request May 30, 2018

Allow CJS ava.config.js #1820

Closed

		@@ -0,0 +1,33 @@
		'use strict';
		const requireEsm = require('esm')(module); // eslint-disable-line no-global-assign

Adds support for ava.config.js files #1761

Adds support for ava.config.js files #1761

Conversation

good-idea commented Apr 2, 2018 • edited by novemberborn Loading

pvdlg commented Apr 3, 2018

novemberborn commented Apr 3, 2018

good-idea commented Apr 3, 2018

novemberborn commented Apr 4, 2018

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

novemberborn commented Apr 8, 2018

good-idea commented Apr 8, 2018

good-idea commented Apr 10, 2018

novemberborn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

good-idea Apr 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

good-idea Apr 22, 2018 • 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

novemberborn commented Apr 15, 2018

good-idea commented Apr 17, 2018

novemberborn commented Apr 18, 2018

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

good-idea commented Apr 30, 2018

novemberborn commented May 7, 2018

novemberborn commented May 20, 2018

good-idea commented May 20, 2018

novemberborn commented May 28, 2018

good-idea commented Apr 2, 2018 •

edited by novemberborn

Loading

good-idea Apr 17, 2018 •

edited

Loading

good-idea Apr 22, 2018 •

edited

Loading