feature-request - add [lazy] option `parents` to fs.writeFile[Sync], fs.appendFile[Sync], fs.open[Sync] #35775

kaizhu256 · 2020-10-23T14:46:56Z

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

this is proof-of-concept to implement feature-request #33559. if feedback is positive/viable, then i will proceed with adding documentation, tests, and other checklist items.

motivation

this feature is intended to improve ergonomics/simplify-scripting-tasks when:

scaffolding new web-projects
creating build-artifacts/coverage-files during ci
cloning website with web-crawler
uploading files to server

in above tasks, user often has no deterministic-knowledge on directory-structure before file-creation.
this feature allows user to lazily create ad-hoc directory-structures as need during file-creation using ergonomic syntax:

fs.writeFileSync(
    "foo/bar/baz/qux.txt",
    "hello world!",
    { parents: true } // will lazily "mkdir --parents" foo/bar/baz as needed
);

performance impact and benchmark

the benchmark (in windows 10) comparing this pr-branch against master-branch shows no-performance-impact on fs.writeFile[Sync] or fs.appendFile[Sync] when option { parents: true } is not used.

when option { parents: true } is enabled:

fs.writeFileSync and fs.appendFileSync is ~10% slower
at lazy-adhoc-directory-creation (vs eager-determistic-directory-creation)
fs.writeFile and fs.appendFile is ~70% slower
at lazy-adhoc-directory-creation (vs eager-determistic-directory-creation)

windows benchmark result

the following results should be reproducible by following benchmark instructions at https://github.com/kaizhu256/node/tree/benchmark.fs.writeFile.mkdirRecursive#run-windows-benchmark

async-operation                                         master-branch           pr-branch       performance-impact
30 * 1000 * (fs.writeFile      w/ no-mkdir      )      934 +/-  50 ms      929 +/-  82 ms    no performance-impact
30 * 1000 * (fs.writeFile      w/ fs.mkdir      )      722 +/- 163 ms      722 +/- 159 ms    no performance-impact
30 * 1000 * (fs.writeFile      w/ parents)             ---- ms     1244 +/- 135 ms    72% slower (lazy-parents vs eager-fs.mkdir)

async-operation                                         master-branch           pr-branch       performance-impact
30 * 1000 * (fs.appendFile     w/ no-mkdir      )      983 +/-  62 ms      966 +/-  92 ms    no performance-impact
30 * 1000 * (fs.appendFile     w/ fs.mkdir      )      739 +/- 175 ms      730 +/- 184 ms    no performance-impact
30 * 1000 * (fs.appendFile     w/ parents)             ---- ms     1252 +/- 126 ms    69% slower (lazy-parents vs eager-fs.mkdir)


 sync-operation                                         master-branch           pr-branch       performance-impact
30 * 1000 * (fs.writeFileSync  w/ no-mkdir      )     1008 +/-  42 ms     1007 +/-  45 ms    no performance-impact
30 * 1000 * (fs.writeFileSync  w/ fs.mkdirSync  )     1643 +/- 121 ms     1611 +/- 117 ms    no performance-impact
30 * 1000 * (fs.writeFileSync  w/ parents)             ---- ms     1791 +/-  87 ms     9% slower (lazy-parents vs eager-fs.mkdir)

 sync-operation                                         master-branch           pr-branch       performance-impact
30 * 1000 * (fs.appendFileSync w/ no-mkdir      )      999 +/-  47 ms     1019 +/-  53 ms    no performance-impact
30 * 1000 * (fs.appendFileSync w/ fs.mkdirSync  )     1617 +/- 103 ms     1619 +/- 109 ms    no performance-impact
30 * 1000 * (fs.appendFileSync w/ parents)             ---- ms     1789 +/- 104 ms    11% slower (lazy-parents vs eager-fs.mkdir)

~~# bikeshed of name mkdirRecursive~~
when scripting the benchmark, i realized the name mkdirRecursive is tedious to type. it also doesn't convey the lazy-nature of this operation, or the -p attribute. am open to other naming suggestions (e.g. { mkdirp: true })
renamed to parents (from unix idiom mkdir --parents)

Trott · 2020-10-23T16:06:56Z

Welcome @kaizhu256 and thanks for the pull request. I'm going to put this into Draft mode while it's being discussed. It will need tests and documentation before we can land it.

Trott · 2020-10-24T12:46:29Z

@nodejs/fs

Trott · 2020-10-25T13:16:06Z

@cjihrig @bcoe

nodejs-github-bot · 2020-11-18T12:28:57Z

CI: https://ci.nodejs.org/job/node-test-pull-request/34450/

kaizhu256 · 2020-11-18T22:50:45Z

@Trott thx for the guidance in my first attempt at a node-pr (regardless whether will cl or not : ) i decided to rename the option from mkdirRecursive to mkdirp, b/c its lazy-behavior differs a bit from fs.mkdir's eager recursive feature.

overall the pr seems mostly complete in substance from my end, and now just awaiting feedback/review.

bcoe · 2020-11-23T23:06:29Z

doc/api/fs.md

@@ -2884,6 +2894,35 @@ Returns an integer representing the file descriptor.
 For detailed information, see the documentation of the asynchronous version of
 this API: [`fs.open()`][].

+## `fs.openWithMkdirp(path[, flags[, mode]], callback)`


If there's not an elegant way to add this functionality to open(), without introducing this new method, I'd rather we start with adding the folder creation option to fs.write and fs.append.

another option is to allow option-bag for 2nd argument: fs.open(path[, options[, mode]]). but yea, being conservative with api is probably best for now. the main motivation is simplifying scripting-chores with fs.writeFile.

I think an options bag would be consistent with the other APIs 👍

bcoe · 2020-11-23T23:07:01Z

doc/api/fs.md

@@ -1419,6 +1427,8 @@ changes:
  * `encoding` {string|null} **Default:** `'utf8'`
  * `mode` {integer} **Default:** `0o666`
  * `flag` {string} See [support of file system `flags`][]. **Default:** `'a'`.
+  * `mkdirp` {boolean} "mkdir -p" directories in `path` if they do not exist.


why don't we call this flag parents (which is what the -p of mkdir) stands for.

good idea. i didn't know it was shorthand for --parent until now.

kaizhu256 · 2021-01-11T05:05:09Z

in latest pr-commit:

expanded scope per @bcoe's suggestion to add options-bag form fs.open(path[, options], callback) and fs.openSync(path[, options]) with accompanying tests and docs
rename option mkdirp to parents.

- fs.open[Sync], - fs.writeFile[Sync], - fs.appendFile[Sync] fs: add new signature-form fs.open(path[, options], callback) fs: add new signature-form fs.openSync(path[, options]) fs: rename option `flags` to `flag` in fs.open for consistency with fs.writeFile[Sync] this feature is intended to improve ergonomics/simplify-scripting when: - creating build-artifacts/coverage-files during ci - scaffolding new web-projects - cloning website with web-crawler allowing user to lazily create ad-hoc directory-structures as need during file-creation with ergonomic syntax: ``` fs.writeFileSync( "foo/bar/baz/qux.txt", "hello world!", { parents: true } // will lazily create parent foo/bar/baz/ as needed ); ``` Fixes: nodejs#33559

bcoe · 2021-01-12T18:11:44Z

nudge to @nodejs/tooling @nodejs/fs for review.

lib/fs.js

…...catch block - remove unnecessary EEXISTS error-check since node v12 - add extra promise test for fs.promises.open()

mcollina · 2021-01-19T07:58:46Z

Can you please amend the description in the PR? The code uses parents.

mcollina

Good work, +1.

mcollina · 2021-01-19T07:59:20Z

doc/api/fs.md

@@ -2805,7 +2815,7 @@ changes:
 -->

 * `path` {string|Buffer|URL}
-* `flags` {string|number} See [support of file system `flags`][].
+* `flag` {string|number} See [support of file system `flags`][].


why was this changed from flags to flag?

To be fair, the FS API and documentation already uses those inconsistently (E.G.: flags in fs.openSync, flag in fs.readFileSync).

Note that changing the documentation from flags to flag would break the existing hypertext links (from StackOverflow, blog posts, etc.), which is something we'd rather not do.

mcollina · 2021-01-19T07:59:31Z

doc/api/fs.md

@@ -2901,7 +2933,7 @@ changes:
 -->

 * `path` {string|Buffer|URL}
-* `flags` {string|number} **Default:** `'r'`.
+* `flag` {string|number} **Default:** `'r'`.


why was this changed from flags to flag?

why was this changed from flags to flag?

i think its lesser evil to alternative -- internal monkey-patching/future-tech-debt converting flag => flags, when called by fs.writeFile[Sync] and fs.appendFile[Sync]

fs.writeFileSync("aa/bb/cc/dd.txt", { // do we really want internal monkey-patching from // `flag` => `flags` when passed to fs.openSync()? flag: "w+", parents: true });

now is the time to change it without backward-compatibility issues.

flags appears in fs.createReadStream(path,{flags}) and fs.createWriteStream(path,{flags}) but those api's aren't as tightly coupled with fs.writeFile[Sync] and fs.appendFile[Sync]

if ppl feel strongly about it, i can revert back to flags and add monkey-patching to play nice with fs.writeFile[Sync] and fs.appendFile[Sync].

I think it should be reverted from this PR as it's not related with the parents option. Maybe you can open another PR focusing on this change? I agree the lack of consistency is kinda bad here, and if we decide to keep the inconsistency, we should at least discuss it thoroughly.

k, reverted documentation of fs.open[Sync]() and fs.promises.open() back to flags in commit ca8deb8.

mcollina · 2021-01-19T08:00:42Z

lib/fs.js

+    mode = options.mode ?? 0o666;
+    parents = options.parents;
+  }
+  flag = stringToFlags(flag);


I think it should be flags everywhere, not flag.

k, added shim converting writeFile[Sync]:flag to open[Sync]:flags in commit 7b5b3e1

async function writeFile(path, data, options) { options = getOptions(options, { encoding: 'utf8', mode: 0o666, flag: 'w' }); - const flag = options.flag || 'w'; + // Don't make changes directly on options object + options = copyObject(options); + // flag to flags shim: + // Resolve inconsistency between writeFile[Sync]:flag to open[Sync]:flags + options.flags = options.flag ?? 'w'; + delete options.flag; ... - const fd = await open(path, flag, options.mode); + const fd = await open(path, options);

mcollina · 2021-01-19T08:01:38Z

lib/fs.js

  callback = makeCallback(callback);

  const req = new FSReqCallback();
-  req.oncomplete = callback;
+  req.oncomplete = (err, fd) => {


If parents is not set, please avoid creating this closure in the first place.

k, added conditional-branch to avoid closure if possible in commit 480eaf7:
is below what was wanted?

diff --git a/lib/fs.js b/lib/fs.js index e5ac3f6e01..28d3f81474 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -481,21 +481,25 @@ function open(path, flag, mode, callback) { callback = makeCallback(callback); const req = new FSReqCallback(); - req.oncomplete = (err, fd) => { - // Lazily create parent-subdirectories if they do not exist - if (err?.code === 'ENOENT' && parents && (flag & O_CREAT)) { - mkdir(pathModule.dirname(path), { recursive: true }, (err2) => { - if (err2) { - callback(err2); - return; - } - // Retry open() after lazily creating parent-subdirectories - open(path, { flag, mode }, callback); - }); - return; - } - callback(err, fd); - }; + if (parents) { + req.oncomplete = (err, fd) => { + // Lazily create parent-subdirectories if they do not exist + if (err?.code === 'ENOENT' && (flag & O_CREAT)) { + mkdir(pathModule.dirname(path), { recursive: true }, (err2) => { + if (err2) { + callback(err2); + return; + } + // Retry open() after lazily creating parent-subdirectories + open(path, { flag, mode }, callback); + }); + return; + } + callback(err, fd); + }; + } else { + req.oncomplete = callback; + } binding.open(pathModule.toNamespacedPath(path), flag,

lib/fs.js

lib/internal/fs/promises.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

…t place.

…t place - update 2

lib/fs.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

bnb · 2021-07-22T06:54:15Z

is there anything I can do to help move this PR forward? It technically Closes #37733 and would generally be a massive DX improvement for those who do this exact operation often.

bcoe · 2021-07-22T15:05:31Z

@bnb @kaizhu256 I'm in support of this feature if the feedback around flags can be addressed, i.e., adding the parents option, but not changing existing parameters.

@kaizhu256 anything we can do to help support this work?

kaizhu256 · 2021-07-23T08:27:08Z

thx for encouragement. revisiting pr, adding a shim converting flag -> flags in fs.writeFile[Sync] and fs.promises.writeFile[Sync] (before calling fs.open) should preserve existing flag/flags naming and documentation.

currently on break, and will get around to updating pr prolly next month.

Resolve inconsistency between writeFile[Sync]:flag to open[Sync]:flags

aduh95

nit: we can get rid of the IIFEs here

test/parallel/test-fs-open-options.js

kaizhu256

add missing tests that fs.promises.[appendFile|writeFile] will disables option mkdirp for flags missing O_CREAT.

test/parallel/test-fs-write-file-with-parents.js

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Mifrill · 2023-11-02T23:07:51Z

lib/fs.js

  }
-  const flagsNumber = stringToFlags(flags);
+  // Handle case where 2nd argument is options-bag
+  if (typeof flags === 'object' && flags) {


If you're only using it in an if statement to check whether flags is an object and you don't need the value of flags itself, then using typeof flags === 'object' is sufficient:

Suggested change

if (typeof flags === 'object' && flags) {

if (typeof flags === 'object') {

That would crash if flags was null, as typeof null === "object"

Mifrill · 2023-11-02T23:08:17Z

lib/fs.js

  path = getValidatedPath(path);
-  const flagsNumber = stringToFlags(flags);
+  // Handle case where 2nd argument is options-bag
+  if (typeof flags === 'object' && flags) {


📓 similar to https://github.com/nodejs/node/pull/35775/files#r1380873544

Suggested change

if (typeof flags === 'object' && flags) {

if (typeof flags === 'object') {

Mifrill · 2023-11-02T23:08:27Z

lib/internal/fs/promises.js

+  let options;
+  let parents;
+  // Handle case where 2nd argument is options-bag
+  if (typeof flags === 'object' && flags) {


📓 similar to https://github.com/nodejs/node/pull/35775/files#r1380873544

Suggested change

if (typeof flags === 'object' && flags) {

if (typeof flags === 'object') {

mcollina · 2024-02-01T07:01:28Z

I'm sorry I've missed this. Is there a way to resurrect this PR? Seems useful.

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 23, 2020

Trott marked this pull request as draft October 23, 2020 16:07

kaizhu256 marked this pull request as ready for review November 16, 2020 10:17

kaizhu256 changed the title ~~feature-request - add [lazy] option mkdirRecursive to fs.writeFile[Sync] and fs.appendFile[Sync]~~ feature-request - add [lazy] option mkdirp to fs.writeFile[Sync], fs.appendFile[Sync], and new function fs.openWithMkdirp[Sync] Nov 16, 2020

kaizhu256 force-pushed the feature.fs.writeFile.mkdirRecursive branch 2 times, most recently from e6e1324 to b8d6132 Compare November 17, 2020 23:10

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2020

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Nov 18, 2020

bcoe requested review from bcoe, devsnek and benjamingr November 23, 2020 22:59

bcoe reviewed Nov 23, 2020

View reviewed changes

kaizhu256 changed the title ~~feature-request - add [lazy] option mkdirp to fs.writeFile[Sync], fs.appendFile[Sync], and new function fs.openWithMkdirp[Sync]~~ feature-request - add [lazy] option parents to fs.writeFile[Sync], fs.appendFile[Sync], fs.open[Sync] Jan 11, 2021

kaizhu256 force-pushed the feature.fs.writeFile.mkdirRecursive branch from a8ebcd3 to 7e0d6b4 Compare January 11, 2021 04:58

kaizhu256 force-pushed the feature.fs.writeFile.mkdirRecursive branch from 16875e2 to 87f66f2 Compare January 12, 2021 06:22

aduh95 reviewed Jan 18, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

- update function fs.openSync to use hasNoEntryError() instead of try…

eacc195

…...catch block - remove unnecessary EEXISTS error-check since node v12 - add extra promise test for fs.promises.open()

mcollina requested changes Jan 19, 2021

View reviewed changes

aduh95 reviewed Jan 19, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

lib/internal/fs/promises.js Outdated Show resolved Hide resolved

kaizhu256 and others added 2 commits January 19, 2021 09:22

Update lib/fs.js

a78c1e2

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Update lib/internal/fs/promises.js

528331a

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

kaizhu256 added 2 commits January 19, 2021 11:56

If parents is not set, please avoid creating this closure in the firs…

480eaf7

…t place.

If parents is not set, please avoid creating this closure in the firs…

ae1a6cf

…t place - update 2

aduh95 reviewed Jan 19, 2021

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

lib/fs.js Outdated Show resolved Hide resolved

kaizhu256 and others added 2 commits January 19, 2021 16:50

Update lib/fs.js

b006217

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

Update lib/fs.js

9129c42

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

kaizhu256 mentioned this pull request Jan 20, 2021

make options.flag[s] consistent across fs.open, fs.writeFile, fs.appendFile, fs.readFile #37004

Open

guybedford force-pushed the master branch from dc5a5da to 8e46568 Compare March 29, 2021 21:33

kaizhu256 added 7 commits August 4, 2021 01:26

resolve conflicts with branch master

b967e5c

revert doc/api/fs.md to branch-master.

43c2b6f

Merge branch 'master' into alpha

a132adb

Update doc/api/fs.md with parents option.

ca8deb8

flag to flags shim:

7b5b3e1

Resolve inconsistency between writeFile[Sync]:flag to open[Sync]:flags

test: fix failing tests

47dbc40

fs: fix eslint errors

8bd103b

aduh95 reviewed Aug 4, 2021

View reviewed changes

test/parallel/test-fs-open-options.js Outdated Show resolved Hide resolved

kaizhu256 commented Aug 4, 2021

View reviewed changes

test/parallel/test-fs-write-file-with-parents.js Outdated Show resolved Hide resolved

test/parallel/test-fs-write-file-with-parents.js Show resolved Hide resolved

test/parallel/test-fs-write-file-with-parents.js Outdated Show resolved Hide resolved

Apply suggestions from code review

229c4e4

Co-authored-by: Antoine du Hamel <duhamelantoine1995@gmail.com>

kaizhu256 requested a review from mcollina August 21, 2021 03:52

Trott force-pushed the main branch from 2d76238 to ca3ed36 Compare November 12, 2022 01:49

Mifrill reviewed Nov 2, 2023

View reviewed changes

	if (typeof flags === 'object' && flags) {
	if (typeof flags === 'object') {

feature-request - add [lazy] option parents to fs.writeFile[Sync], fs.appendFile[Sync], fs.open[Sync] #35775

Are you sure you want to change the base?

feature-request - add [lazy] option parents to fs.writeFile[Sync], fs.appendFile[Sync], fs.open[Sync] #35775

Conversation

kaizhu256 commented Oct 23, 2020 • edited Loading

Checklist

motivation

performance impact and benchmark

windows benchmark result

Trott commented Oct 23, 2020

Trott commented Oct 24, 2020

Trott commented Oct 25, 2020

nodejs-github-bot commented Nov 18, 2020

kaizhu256 commented Nov 18, 2020

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

kaizhu256 commented Jan 11, 2021

bcoe commented Jan 12, 2021

mcollina commented Jan 19, 2021

mcollina 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

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

kaizhu256 Jan 19, 2021 • edited Loading

Choose a reason for hiding this comment

bnb commented Jul 22, 2021

bcoe commented Jul 22, 2021

kaizhu256 commented Jul 23, 2021

aduh95 left a comment

Choose a reason for hiding this comment

kaizhu256 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

mcollina commented Feb 1, 2024

feature-request - add [lazy] option `parents` to fs.writeFile[Sync], fs.appendFile[Sync], fs.open[Sync] #35775

feature-request - add [lazy] option `parents` to fs.writeFile[Sync], fs.appendFile[Sync], fs.open[Sync] #35775

kaizhu256 commented Oct 23, 2020 •

edited

Loading

kaizhu256 Jan 19, 2021 •

edited

Loading