@seishun
Should we ignore that v4.* does not support default parameters, so this fix cannot be backported?

v4 is heading into maintenance mode in just over a week. We shouldn't need to worry about the inability to backport. The real concern, however, is that param defaults are not very optimized in v6 or v7. They likely won't see significant performance improvement until the new ignition toolchain lands in v8 5.9.

@seishun ... have you benchmarked these changes at all?

Never written benchmarks before, so I just copied readfile.js and changed read into write. Results:

                                                   improvement confidence   p.value
 fs\\writefile.js concurrent=1 len=1024 dur=5          -0.10 %            0.9584959
 fs\\writefile.js concurrent=1 len=16777216 dur=5      -0.25 %            0.8447709
 fs\\writefile.js concurrent=10 len=1024 dur=5          0.67 %            0.4847459
 fs\\writefile.js concurrent=10 len=16777216 dur=5     -0.70 %            0.5841252

No difference basically.

Tried replacing the default argument with || options, there was no difference either:

Saving 7 x 7 in image
                                                   improvement confidence   p.value
 fs\\writefile.js concurrent=1 len=1024 dur=5           0.48 %            0.7482698
 fs\\writefile.js concurrent=1 len=16777216 dur=5      -1.39 %            0.2153722
 fs\\writefile.js concurrent=10 len=1024 dur=5         -0.47 %            0.5425692
 fs\\writefile.js concurrent=10 len=16777216 dur=5      1.07 %            0.4011331

@vsemozhetbyt can you comment? Maybe I'm doing something wrong.

@seishun IIRC, we try to fix it not for performance, but for API correctness, so there can be no difference in benchmarks.

@vsemozhetbyt Yes, but your PR caused a performance degradation, and mine apparently doesn't. Which is what confuses me.

The key thing with the benchmarks is that we don't want to see a significant loss. It's perfectly fine for things to remain the same :-)

@seishun ... which versions were you comparing inn the benchmark runs above? I'd like to do a comparison run on my end

@seishun That PR performance degradation was murky and I did not understand it. However, we changed things differently, so v8 may like your variant more than mine)

@vsemozhetbyt I tried your variant too, but there was no difference.

Now I'm more concerned about consistency. It seems we don't use default arguments anywhere in node.js, so maybe we shouldn't deviate from that here either. @nodejs/collaborators thoughts?

Also, do we want to add this silly benchmark (last commit here)?

@seishun Did you try with your benchmark or my ones (there are two there: with concurrent writes in the commit and with linear writes in this comment)? Maybe it is something wrong with my benchmarks.

@vsemozhetbyt There's a difference with your benchmark.

With a default argument:

                                                                                 improvement confidence      p.value
 fs\\read-write-file-params.js withOptions="false" methodName="writeFile" n=1000    -13.02 %        *** 1.215164e-09
 fs\\read-write-file-params.js withOptions="true" methodName="writeFile" n=1000     -12.72 %        *** 1.456396e-08

With || options:

                                                                                 improvement confidence      p.value
 fs\\read-write-file-params.js withOptions="false" methodName="writeFile" n=1000    -10.62 %        *** 1.591341e-06
 fs\\read-write-file-params.js withOptions="true" methodName="writeFile" n=1000     -13.88 %        *** 4.443414e-09

The difference is that your benchmark doesn't wait for the callback, which I guess makes it less realistic than mine.

@jasnell

@seishun ... which versions were you comparing inn the benchmark runs above? I'd like to do a comparison run on my end

I compared master and this PR rebased on master.

@seishun If I get it right, the benchmark with concurrent writes (in the commit) doesn't use callbacks, while the benchmark with linear writes (in this comment) does use callbacks. However, I had downgrade with both(

Now I'm more concerned about consistency. It seems we don't use default arguments anywhere in node.js, so maybe we shouldn't deviate from that here either. @nodejs/collaborators thoughts?

If it benchmarks okay (which it seems to by now?) I’m in favour of using default arguments, most of the time it increases readability.

Default arguments seem to have improved significantly recently but I'd would still likely avoid them in hot code for a bit longer.

Also keep in mind that there are other side effects... Such as the 'length' property of the function not counting params with defaults.

How can we proceed with this fix?

Ahum, I'm slightly towards -1 on this. Not because of the change in itself, but because writeFile requires data to be a string, a buffer or a uint8array. I think we should check that, and throw an error if that's not the case.
To me, this PR is ok, but it does not fix the problem in the description, in fact it makes it even less clear.

To me, this PR is ok, but it does not fix the problem in the description, in fact it makes it even less clear.

Please provide a test case that isn't fixed by this PR.

To me,

> fs.writeFile('foo', console.log)
undefined
> (node:13028) [DEP0013] DeprecationWarning: Calling an asynchronous function without callback is deprecated.

Should throw because of a missing data object, not give a warning because of a missing callback. The missing callback warning should still be there, in case we call writeFile(path, data). This PR is not improving the ergonomics of API: the user clearly did not put a valid data object in (string, data, Uint8Array), and we should throw on that, not give a warning about a missing callback.

@mcollina It's true that node allows you to specify any value as data:

C:\Users\Nikolai\Downloads\node>node
> fs.writeFile('foo', console.log, function() {});
undefined
>
(To exit, press ^C again or type .exit)
>

C:\Users\Nikolai\Downloads\node>cat foo
function () { [native code] }

But I think it's a separate issue, and it's not clear cut whether we want to be more strict about it.

The problem in:

> fs.writeFile('foo', console.log)
undefined
> (node:13028) [DEP0013] DeprecationWarning: Calling an asynchronous function without callback is deprecated.

is that console.log is a function, which can be used as a callback. So, as a user I would think that I did put the callback in there. So, to fix this I think we should be more strict and require that what we write is either a String, UInt8Array, or Buffer.

So, to fix this I think we should be more strict and require that what we write is either a String, UInt8Array, or Buffer.

This automatic conversion goes way back to fa829b0 and nodejs/node-v0.x-archive#657. I'm not sure if it's something worth changing at this point.

@nodejs/collaborators opinions?

It turns out that @mscdex has already done part of the work in e8a4290. So this PR will just make things consistent between fs.writeFile and fs.appendFile.

Since this asserts https://nodejs.org/api/fs.html#fs_fs_writefile_file_data_options_callback I think an assert.throws test case should be added to test/parallel/test-fs-write-file.js and test/parallel/test-fs-append-file.js

@refack what test case exactly? It doesn't introduce any new exceptions.

@refack what test case exactly? It doesn't introduce any new exceptions.

Forgot we reverted throwing in #12976, but it should emit warning
(node:15000) [DEP0013] DeprecationWarning: Calling an asynchronous function without callback is deprecated.

Added a test, PTAL.

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

The failures seem unrelated?

OSX - known #13559
AIX - known #13577

freeBSD - parallel/test-process-external-stdio-close
Maybe so rerunning...
https://ci.nodejs.org/job/node-test-commit-freebsd/9644/

freeBSD was a flake - #13586

Should this be backported to v6.x?

If #12456 was backported, then this should be too.