fs: implement recursive (mkdirp) functionality #21875

bcoe · 2018-07-19T00:41:02Z

I'm an advocate of a small core library (and this is a big part of why such an amazing ecosystem of modules has grown up around Node.js and JavaScript).

Having said this, I love the Tooling Group initiative being advocated by @boneskull; some modules are so prolific, and the behavior at this point in Node's history so standardized, that the functionality should probably simply be part of the core API.

Two great examples of this are mkdirp and rimraf.

mkdirp and rimraf get downloaded over 8,000,000 times a day, and are dependencies of some 250,000 applications on GitHub -- wow!.

This pull request implements mkdirp by adding a parent (make the parents) boolean option to mkdir, mkdirSync, and promises.mkdir methods.

I've opted to implement the feature in C++ mainly because the codepath wasn't shared for mkdir and promises.mkdir and this felt like a good way to DRY things up a bit.

Interested to hear what people think, and excited for future discussions around Node tooling.

CC: @stefanpenner, @cb1kenobi.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-07-19T00:41:04Z

@bcoe build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/310/pipeline

rubys · 2018-07-19T01:31:14Z

doc/api/fs.md

{Object|integer}

applies also to fs.mkdirSync and fsPromises.mkdir

It's not clear that the integer value is still supported. It should.

updating to rubys' suggestion of {Object|integer}, this was copy pasta on my part.

stefanpenner · 2018-07-19T05:33:42Z

The design phase may be over, so I may have missed the boat. But I do find it surprising this implemented as a flag to mkdir and not fs.mkdirp and fs.mkdirpSync.

This may just be familiarity with the existing patterns. But an explicit method, does have some value:

easy to feature detect (for us library authors that need/want to support a wide range of node versions).
easy to transition, simply do what the fs-extra users do today.
easy to discover. fs.<tab> would reveal
IMHO less error prone, specifically mode -> pojo or number, plus potential typo's to the arguments.

Ultimately, If I have to choose between it not existing, and being implemented as a flag. I would absolutely choose it existing as a flag. But I do believe my preference, if given the option, would be as a separate method.

stefanpenner · 2018-07-19T05:34:05Z

I would be curious, how this implementation compares performance wise to mkdirp today.

stefanpenner · 2018-07-19T05:34:20Z

Also, I am super excited to see this be baked in!

cb1kenobi · 2018-07-19T05:40:21Z

I agree with @stefanpenner in that I find fs.mkdirp and fs.mkdirpSync more favorable.

If you continue down the road of parent being a flag, why not default it to true? Is there a real world scenario where creating the parent directories is a bad thing?

joyeecheung · 2018-07-19T05:52:37Z

Setting whether this should be in core aside (I am neural on this given how many times I need this in the user land), a few notes on the current design/implementation:

It's not ideal to overload the second argument offs.mkdir/fs.mkdirSync. Separating that to a new API would simplify things a lot.
Why is it implemented in C++? Are there any performance benefits? node-mkdirp is implemented entirely in JS and it seems cleaner than this implementation, which also brings another question...
Why can't we just put node-mkdirp in deps (of course, if the original author agrees with it), or just copy that code and put a license header like what we did with punycode? Surely users would prefer const mkdirp = fs.mkdirp over manually upgrading to a new API? (though mkdirp is not an ideal name)

joyeecheung · 2018-07-19T06:13:54Z

Sorry, missed the bits about C++ implementation in the OP

I've opted to implement the feature in C++ mainly because the codepath wasn't shared for mkdir and promises.mkdir and this felt like a good way to DRY things up a bit.

If I understand correctly this just DRYs the validation (but only if this is implemented as an option of fs.mkdir)? We tend to implement extensions to fs using existing functions (e.g. fs.exists uses fs.access, fs.appendFile uses fs.fs.writeFile, fs.writeFile uses fs.open and fs.write.etc), so it doesn't seem to make sense to make an exception for mkdirp since there are no additional libuv routines for it and it's entirely implementable in JS.

EDIT: as for promises it should be fine if we just promisify the callback version?

sindresorhus · 2018-07-19T06:22:30Z

I'm the author of make-dir (has half the daily downloads of mkdirp) and I fully support this. It's such a basic primitive that I need in almost every project. It would be great to have this in core.

I don't think it should be a new method. The difference between mkdir and mkdirp when reading code would be easy to miss. I also don't think the current option name parent is clear enough. It doesn't convey what it actually does. Some suggestions: createIntermediates (This is what Cocoa uses), createParents, createParentDirectories.

I would be happy to donate make-dir to the Node.js project if you decide to implement it in pure JS.

addaleax · 2018-07-19T10:02:34Z

src/node_file.cc

This has become an unrelated whitespace change :)

addaleax · 2018-07-19T10:04:28Z

src/node_file.cc

I think the compiler could optimize the copy operation away here, but either way, it’s a bit more idiomatic to initialize this as FSContinuationData continuation_data(req, mode, cb); (i.e. without the assignment) 🙂

addaleax · 2018-07-19T10:05:20Z

src/node_file.cc

style nit: new line for default (and/or just omit the break; and the braces here)

addaleax · 2018-07-19T10:09:44Z

src/node_file.cc

I think this will lead to an infinite loop if there is no / or \ character?

This can happen if the current working directory does not exist anymore:

const fs = require('fs'); fs.mkdirSync('/tmp/foo'); process.chdir('/tmp/foo'); fs.rmdirSync('/tmp/foo'); fs.mkdirSync('X', { parent: true });

added a test case for this.

addaleax · 2018-07-19T10:12:58Z

src/node_file.cc

I just landed #21839 – can you use FSReqBase::from_req(req) here and below? :)

bcoe · 2018-07-26T21:26:10Z

src/node_file.cc

we discussed out of band the problem with using req_wrap->Dispatch() and @addaleax and I came the the decision that using uv_fs_mkdir directly was sufficient.

I think we need to call uv_fs_req_cleanup() after a uv_fs_mkdir call is done, but before we start the next one?

I'm now calling uv_fs_req_cleanup() for all but the last fs operation, since it's called in the codebase already at the top level of the request wrap.

addaleax · 2018-07-19T10:20:50Z

src/node_file.h

tiny style nit: We usually leave blank lines between methods

addaleax · 2018-07-19T10:23:18Z

src/node_file.h

I think everywhere where this is called, the path parameter is not used afterwards in the caller. Currently, this creates a copy of path and then disposes the original – it might make sense to use std::string&& as the parameter and move the string rather than copying it? (i.e. use paths.emplace_back(std::move(path)); here and continuation_data->PushPath(std::move(str)); elsewhere)

addaleax · 2018-07-19T10:23:38Z

src/node_file.h

Similarly, this also creates a copy but doesn’t really need to – I think path = std::move(paths.back()) works?

jasnell · 2018-07-19T13:42:18Z

doc/api/fs.md

nit: there should be backticks around options here.

jasnell · 2018-07-19T13:42:39Z

doc/api/fs.md

An example showing each variant would be nice.

jasnell · 2018-07-19T13:43:13Z

doc/api/fs.md

Backticks around options here also.

jasnell · 2018-07-19T13:45:15Z

lib/fs.js

likely should be 'options.parent' for the first argument here

jasnell · 2018-07-19T13:47:03Z

src/node_file.cc

extreme nit: We generally put the body of if statements on a new line or within brackets.

jasnell · 2018-07-19T13:49:00Z

Great to see this. Would like @addaleax's nits addressed then will sign off :-)

vsemozhetbyt · 2018-07-19T15:39:42Z

If the current design is preserved.

When we change a heading in the docs, we need to check if there are links to this outdated heading (to do so, copy the old hash from the old HTML doc and grep this hash in *.md sources).

Currently, this fragment needs to be updated:

https://github.com/nodejs/node/blame/6a99e3e21c5ce13eb079c5d29db4d4fe616802fc/doc/api/fs.md#L4630

vsemozhetbyt · 2018-07-19T15:48:21Z

doc/api/fs.md

A tiny nit: we usually add periods after the **Default:** ... parts only if they followed full sentences (the same for changes below).

mhdawson · 2018-07-19T16:32:32Z

test/parallel/test-fs-mkdir.js

Should this be something like ${tmpdir.path}/testX/testY/../

or even

${tmpdir.path}/testX/../testX/testY/

to make sure the directory created is under the temporary directory that gets cleaned up?

so that we know its creating

I've updated both the ../ and ./ test accordingly, found a couple bugs in the process.

mhdawson · 2018-07-19T16:35:03Z

test/parallel/test-fs-promises.js

Should we have the ../ variant here as well.

mhdawson · 2018-07-19T16:35:50Z

Overall happy to see this, will sign off once comments from everybody so far have been addressed.

Trott · 2018-07-19T20:46:07Z

@nodejs/fs

ronkorving · 2018-07-20T02:21:15Z

@bcoe I'm all in favor of making this happen. Was there a public design phase by the way? I don't mind at all PRs coming in without it having been publicly discussed first, but now that it's here, the merits of one API over another should be discussed (if it hasn't happened yet). Could you link to some existing conversation if it exists?

I'm with @joyeecheung that a pure JavaScript implementation would make sense, although performance may be reason to make an exception. A demonstration of performance would be nice; how does this implementation perform vs. a pure JS implementation that does the same operations?

Trott · 2018-09-24T23:16:09Z

Based on nodejs/TSC#604, I'm removing the dont-land-on-v10.x label. This can land in the next v10.x release. /cc @targos

Implements mkdirp functionality in node_file.cc. The Benefit of implementing in C++ layer is that the logic is more easily shared between the Promise and callback implementation and there are notable performance improvements. This commit is part of the Tooling Group Initiative. Refs: nodejs/user-feedback#70 PR-URL: #21875 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: Ron Korving <ron@ronkorving.nl> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Sam Ruby <rubys@intertwingly.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

PR-URL: #22949 Refs: #21875 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>

targos · 2018-09-25T07:04:32Z

Landed this and all others from #21875 (comment).

Notable changes: * assert * The diff output is now a tiny bit improved by sorting object properties when inspecting the values that are compared with each other. #22788 * cli * The options parser now normalizes `_` to `-` in all multi-word command-line flags, e.g. `--no_warnings` has the same effect as `--no-warnings`. #23020 * Added bash completion for the `node` binary. To generate a bash completion script, run `node --completion-bash`. The output can be saved to a file which can be sourced to enable completion. #20713 * crypto * Added support for PEM-level encryption. #23151 * Added an API asymmetric key pair generation. The new methods `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be used to generate public and private key pairs. The API supports RSA, DSA and EC and a variety of key encodings (both PEM and DER). #22660 * fs * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If this option is set to true, non-existing parent folders will be automatically created. #21875 * http2 * Added a `'ping'` event to `Http2Session` that is emitted whenever a non-ack `PING` is received. #23009 * Added support for the `ORIGIN` frame. #22956 * module * Added `module.createRequireFromPath(filename)`. This new method can be used to create a custom require function that will resolve modules relative to the filename path. #19360 * process * Added a `'multipleResolves'` process event that is emitted whenever a `Promise` is attempted to be resolved multiple times, e.g. if the `resolve` and `reject` functions are both called in a `Promise` executor. #22218 * **url** * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These methods can be used to correctly convert between file: URLs and absolute paths. #22506 * **util** * Added the `sorted` option to `util.inspect()`. If set to `true`, all properties of an object and Set and Map entries will be sorted in the returned string. If set to a function, it is used as a compare function. #22788 * The `util.instpect.custom` symbol is now defined in the global symbol registry as `Symbol.for('nodejs.util.inspect.custom')`. #20857 * **Windows** * The Windows msi installer now provides an option to automatically install the tools required to build native modules. #22645 * **Added new collaborators**: * digitalinfinity - Hitesh Kanwathirtha PR-URL: #23313

Notable changes: * assert * The diff output is now a tiny bit improved by sorting object properties when inspecting the values that are compared with each other. #22788 * cli * The options parser now normalizes `_` to `-` in all multi-word command-line flags, e.g. `--no_warnings` has the same effect as `--no-warnings`. #23020 * Added bash completion for the `node` binary. To generate a bash completion script, run `node --completion-bash`. The output can be saved to a file which can be sourced to enable completion. #20713 * crypto * Added support for PEM-level encryption. #23151 * Added an API asymmetric key pair generation. The new methods `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be used to generate public and private key pairs. The API supports RSA, DSA and EC and a variety of key encodings (both PEM and DER). #22660 * fs * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If this option is set to true, non-existing parent folders will be automatically created. #21875 * http2 * Added a `'ping'` event to `Http2Session` that is emitted whenever a non-ack `PING` is received. #23009 * Added support for the `ORIGIN` frame. #22956 * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect protocol support to allow use of WebSockets over HTTP/2. #23284 * module * Added `module.createRequireFromPath(filename)`. This new method can be used to create a custom require function that will resolve modules relative to the filename path. #19360 * process * Added a `'multipleResolves'` process event that is emitted whenever a `Promise` is attempted to be resolved multiple times, e.g. if the `resolve` and `reject` functions are both called in a `Promise` executor. #22218 * url * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These methods can be used to correctly convert between file: URLs and absolute paths. #22506 * util * Added the `sorted` option to `util.inspect()`. If set to `true`, all properties of an object and Set and Map entries will be sorted in the returned string. If set to a function, it is used as a compare function. #22788 * The `util.instpect.custom` symbol is now defined in the global symbol registry as `Symbol.for('nodejs.util.inspect.custom')`. #20857 * Added support for `BigInt` numbers in `util.format()`. #22097 * V8 API * A number of V8 C++ APIs have been marked as deprecated since they have been removed in the upstream repository. Replacement APIs are added where necessary. #23159 * Windows * The Windows msi installer now provides an option to automatically install the tools required to build native modules. #22645 * Workers * Debugging support for Workers using the DevTools protocol has been implemented. #21364 * The public `inspector` module is now enabled in Workers. #22769 * Added new collaborators: * digitalinfinity - Hitesh Kanwathirtha PR-URL: #23313

bcoe added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. labels Jul 19, 2018

bcoe requested review from addaleax and mhdawson July 19, 2018 00:41

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Jul 19, 2018

rubys reviewed Jul 19, 2018

View reviewed changes

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Jul 19, 2018

addaleax approved these changes Jul 19, 2018

View reviewed changes