3.0 release

[sudo-suhas]

Changelog

• Setup pre-commit hook using husky with lint-staged
• Setup prettier using eslint-plugin-prettier with eslint-config-prettier
• Run all files through prettier
• Drop node 0.12 support, use typicode/please-upgrade-node for helpful message before exiting
• Remove node-version-check from deps
• Tweak eslint settings
• ES6ify - () => {}, const/let , ${template} strings
• createExplorer.js
  • Add func resolveDir to check if given path is a dir and return parent dir if not(sync or async mode supported).
  • Use the same identity func for both sync and async.
• loadRc.js - Add func makeRcFileParser to create a rc file parser for the given parse func, extension and the next extension.
• Remove object-assign from deps.
• Move utility funcs statStubIsDirectory, makeReadFileSyncStub, assertSearchSequence,used for tests , to util.js
• Migrate tests to jest
• Remove unused dev-dependencies expect, lodash, nyc, sinon, tap-spec and tape.

TODO

• pre-commit + lint-staged + prettier
• Drop node 0.12 support
• ES6ify
• Handle empty config file edge case
• Migrate tests to jest

Closes #71

[sudo-suhas]

@davidtheclark What should the behavior when cosmiconfig gets an empty config file? Right now, the behavior is varied for different file formats and whether expected format was specified:

File format	Format Specified	Threw Error	Error Message
JSON	✘	✔	Error: Failed to parse
JSON	✓	✔	JSONError: No data, empty input
YAML	✘	✔	Error: Failed to parse
YAML	✓	✔	Error: Failed to parse
JS	✘	✔	Error: Failed to parse
JS	✓	❌	--

I think we should exit early with either an error or return null from loadDefinedFile>parseContent if content is falsy.

[davidtheclark]

@sudo-suhas: These first two commits look good (ea485ed and 1f372ab). 82d5b79 does too much, though. I would have much preferred if the step of ES6ifying the code was a separate commit, because with all the formatting changes it's not possible review refactoring changes that you also made. If you're confident it's going to work fine, we can go with it — but I can't really review it.

What should the behavior when cosmiconfig gets an empty config file?

So the file exists but has no content. Your idea of adding a more friendly error message sounds good 👍 . I'd suggest separating it from this PR, since it's a new feature instead of a code style & convention update.

[sudo-suhas]

82d5b79 does too much, though.

I have split and rebased it into separate commits. While I am reasonable confident with the changes(tests passed), it would be better if it were reviewed as well.

I'd suggest separating it from this PR, since it's a new feature instead of a code style & convention update.

Though it is unlikely to impact anyone, technically this is a breaking change and that is why I brought it up here..

[davidtheclark]

Thanks for splitting the commits! I'm going to ask that you split one more time, though, for safety. I think it's safer to refactor the code first without changing the tests, then refactor the tests without changing the code. That way you know you didn't introduce a bug in your code refactor that your test refactor swallowed. If we can do that, I'll be satisfied :)

All the changes look great!

Though it is unlikely to impact anyone, technically this is a breaking change and that is why I brought it up here..

Ah, yes. Makes sense.

[sudo-suhas]

I'm going to ask that you split one more time, though, for safety

Done 👍

Hopefully we can finish the rest of the refactoring by next week end.

Edit: Did I misunderstand what you meant by one more split? Did you want me to split the ES6 refactoring commit for lib and test?

[davidtheclark]

@sudo-suhas: Excellent! The branch looks great so far.

[sudo-suhas]

I usually have the object-shorthand rule enabled in eslint. I didn't realise that this was supported in node 4. Unless you prefer to not use it, I'll squash this with the earlier commit refactor: ES6ify.

[sudo-suhas]

@davidtheclark, need a little help. I have migrated the test file successful-files.test.js to jest and I have introduced a couple of helpers to easily write tests without having to repeat code for sync and async. I have created a gist with the modified file - https://gist.github.com/sudo-suhas/3d84fa23ae4cba5700c916868fe7fa38. I want your opinion on this. Also, what do you think of using babel-jest?

[davidtheclark]

@sudo-suhas: I'm not a big fan of the testResolves and testRejects helpers, because they obscure the procedure of the test so would make it more difficult to debug unexpected outcomes. I myself would prefer repeating the sync-async pattern.

Also, what do you think of using babel-jest?

My preference is to use the same syntax (and same linting) in the tests as in the codebase.

[sudo-suhas]

Alright. I tend to take the dedupe thing a bit too far sometimes 😅. Please don't mind but what do you think of a middle ground? A simple function which just calls the given test function once in sync and once in async.

function testSyncAndAsync(name, testFn) {
  describe('sync', () => {
    it(name, testFn(true));
  });

  describe('async', () => {
    it(name, testFn(false));
  });
}

The differences in sync and async mode could be handled using funcRunner in most cases.

[davidtheclark]

👋 @sudo-suhas sorry this feel off my radar. I was visiting extended family. I will look at it soon!

[sudo-suhas]

No problem at all 😄.

Edit: I did a small commit amend for unneeded jest config in package.json:

"jest": {
    "testEnvironment": "node",
-    "testPathIgnorePatterns": [
-      "/node_modules/",
-      "/_bkp_test/"
-    ]
  }

[davidtheclark]

@sudo-suhas: Seems ok to me 👌 . Is there anything in particular you wanted me to look at?

[sudo-suhas]

Great. One of the things I wanted to know if you were okay with the helpers testSyncAndAsync and testFuncsRunner. Other than that, nothing in particular, but I value your opinion. So shall we release 3.0 then?

[davidtheclark]

@sudo-suhas: Yeah, I'm a little wary of how much complexity there is in the tests now, making them more difficult to read and add to — but you've invested the time here, so if you think it's worthwhile, I'll take your word for it. Probably it makes plenty of sense after you write a few tests with the framework 👍

Am I right in thinking that 3.0 makes only one intentional change — introducing sync mode — but otherwise everything should work the same?

[sudo-suhas]

@davidtheclark I understand that we should strike the right balance wrt complexity and ease of writing a test(mainly repetition of similar steps). If you feel that we need to drop the helpers, I am more than willing to refactor all the required tests. Please take another look.

Actually, there are a couple of other changes as well. The most important of which is dropping support for node < 4. Additionally, an error will be thrown on encountering an empty config file.

[davidtheclark]

Dropping support for Node 0.12 seems totally fine with me.

Also, I like the empty config file error.

Regarding test helpers — let's just go with what you wrote and see how it works! If we or contributors have problems with it, we can always refactor later. Does that sound good?

[sudo-suhas]

That sounds great 👍

[davidtheclark]

Oooooooh, the latest Prettier release uses cosmiconfig! https://github.com/prettier/prettier#configuration-file

That means we have another big user, like stylelint, that we probably should double-check against before releasing this branch, @sudo-suhas 😬

[sudo-suhas]

That.. is pretty cosmic 😁.

I remember we discussed integration tests. I had been meaning to ask you about it. So here's what I have so far. Use jest#mock to mock cosmiconfig and invoke prettier/stylelint programmatically. Either have a simple test to see that no errors are thrown or use snapshots to have a more through check. I don't really have any experience writing integration tests so I could use some help here.

[davidtheclark]

@sudo-suhas A few thoughts came to mind about this branch:

• Can we integrate Prettier with lint-staged instead of with the ESLint plugin? I really like that workflow, where you don't have to remember to format the files yourself.
• I'm thinking maybe we should not start erroring when we find an empty config file. We could avoid this breaking change and leave that to the consumer: they can always decide whether to error or not. I just feel like it's an opinion that this module doesn't need to have.
• If your method for automated integration tests with jest#mock works, that sounds like a great way to do it! I don't have experience testing this kind of thing, either — my default would just be to perform manual tests when I suspect danger.

[sudo-suhas]

• Can we integrate Prettier with lint-staged instead of with the ESLint plugin? I really like that workflow, where you don't have to remember to format the files yourself.

I can do that, no problem. Though, even now, remembering to format is not required as this is taken care by eslint --fix in lint-staged but eslint will keep showing errors while in development if not formatted correctly. Another thing is that few of the eslint rules conflict with prettier formatting(arrow-parens, no-confusing-arrow). So I would prefer to extend eslint-config-prettier even if we remove eslint-plugin-prettier.

• I'm thinking maybe we should not start erroring when we find an empty config file.

As you can see in #78 (comment), we already throw errors for empty files in almost all cases. We would just be throwing a more accurate error instead of different ones for different file formats. Are you sure you want me to remove this check?

[davidtheclark]

@sudo-suhas: Ok, good decisions 👍

[sudo-suhas]

@davidtheclark I have rebased and removed eslint-plugin-prettier in cad5e4f. lint-staged now uses prettier itself. I also snuck in a minor improvement to funcRunner in 9d4f761 using Array#reduce. Can we please merge this in? I'll tackle integration tests in a separate PR. That way, we can go ahead with #79 as well.

[davidtheclark]

Merged!