Use native recursive option when available/appropriate

[coreyfarrell]

I've split into 3 commits to make it easier if you do not want all changes. First commit just fixes testing to work with ava@1. Second comment enables use of native recursive fs.mkdir but causes really low coverage to be reported when tested against node.js >=10.12.0. The third commit modifies testing so the legacy methods are used even in current versions of node. I'm not sure it's a good idea to chase coverage like this, in node 10.12.0 we should probably test error handling of the native method does the same as the legacy method?

Test coverage on >=10.12.0 without 3rd commit:

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |    53.57 |    35.71 |    41.67 |     53.7 |                   |
 index.js |    53.57 |    35.71 |    41.67 |     53.7 |... 98,101,105,108 |
----------|----------|----------|----------|----------|-------------------|

With fs option custom test added but error handling tests not modified:

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |    83.93 |    71.43 |    83.33 |    83.33 |                   |
 index.js |    83.93 |    71.43 |    83.33 |    83.33 |... 9,96,97,98,101 |
----------|----------|----------|----------|----------|-------------------|

With complete third commit:

----------|----------|----------|----------|----------|-------------------|
File      |  % Stmts | % Branch |  % Funcs |  % Lines | Uncovered Line #s |
----------|----------|----------|----------|----------|-------------------|
All files |    96.43 |    85.71 |      100 |     96.3 |                   |
 index.js |    96.43 |    85.71 |      100 |     96.3 |             51,89 |
----------|----------|----------|----------|----------|-------------------|

---

Closes #5

[coreyfarrell]

Apparently windows throws EPERM on fs.mkdir('/') but then make-dir legacy method suppresses the error since that path already exists.

Another thing I've noticed is that neither fs.mkdir nor make-dir throw an error if you request a different mode than the existing path (tested in Linux with sync and async). So makeDir('/', {mode: 0o777}) will report success but will not fs.chmod('/', 0o777).

make-dir#master is already dropping node.js v4 so it's already going to be a semver-major bump to 2.x, please let me know how you'd prefer these issues be addressed.

[sindresorhus]

The third commit modifies testing so the legacy methods are used even in current versions of node. I'm not sure it's a good idea to chase coverage like this

It's not. You can force Nyc to ignore it using a directive comment. Example: https://github.com/sindresorhus/got/blob/c8e358ff3642fe169ae947efd29db8a418b12dc1/source/utils/timed-out.js#L18

[sindresorhus]

Apparently windows throws EPERM on fs.mkdir('/') but then make-dir legacy method suppresses the error since that path already exists.

We should handle it the same as whatever the native fs.mkdir('/', {recursive: true}) does.

Another thing I've noticed is that neither fs.mkdir nor make-dir throw an error if you request a different mode than the existing path (tested in Linux with sync and async). So makeDir('/', {mode: 0o777}) will report success but will not fs.chmod('/', 0o777).

Let's follow whatever fs.mkdir does here. It might be a good reason or an historical reason why it does that. If you think it's the wrong behavior, can you open an issue on Node.js first?

[sindresorhus]

Can you update the docs to say that it uses the native method when available in Node.js and link to the recursive option docs and when it's not using a custom fs implementation.

[coreyfarrell]

I'm aware of the istanbul ignore directive but we do want that coverage when testing against older versions of node. So maybe just adding the fs options custom test is the best way. It won't have great coverage for >=10.12.0 but we'll still verify coverage for older versions.

I've opened nodejs/node#25110 to get clarification on the intended functionality of recursive mkdir. If this causes a fix to node.js then I'll probably update the semver check to ensure that only fixed versions of the native mkdir are used.

I'll post updates including docs once I hear back on the node.js issue.

[coreyfarrell]

After discussing with @bcoe I believe the current behavior of the native fs.mkdir is correct so I've posted nodejs/node#25340 to note the platform difference for /. The fact that it does not set permissions for existing folders is correct since man 2 mkdir specifies that the mode is only for newly created directories, mkdir -p command-line utility works the same way.

[coreyfarrell]

I'm booting up a Win10 VM so I can troubleshoot handles non-existent root. I'm updating the tests to match behavior of node.js 10. I'm not sure why the native method provides a different error.message for fs.mkdir vs fs.mkdirSync, I'm just going to exclude checking for the full path from the message regex.

[coreyfarrell]

Sorry for the noise, testing on Windows is difficult for me. I think I've got this so make-dir have the same behavior with or without the native recursive option.

[coreyfarrell]

@sindresorhus when you have a chance could you give this a review?

[sindresorhus]

Looks good :)