GeezerVison

I don't think it's that easy, especially since there are 3 consecutive integer parameters and trying to determine which was specified if there were less than 3 integers passed is impossible. I think we'd only be able to (reliably) support the following cases:

fs.read(fd, buffer, offset, length, position, callback);
fs.read(fd, buffer, callback);
fs.read(fd, callback);

For the last signature, the documentation must be updated to clearly indicate the default buffer size.

Additionally, I think the language features used here may cause performance issues so it'd be better to just stick to the traditional type checking.

I don't think it's that easy, especially since there are 3 consecutive integer parameters and trying to determine which was specified if there were less than 3 integers passed is impossible.

I see what you are saying, but i think the assumption is(and maybe it is just my assumption), that when you want to specify, for example length, that you would also need to specify any params before it. So we should know what param is being passed in since they are positional

So in that case the signatures would look like this:

fs.read(fd, buffer, offset, length, position, callback);
fs.read(fd, buffer, offset, length, callback);
fs.read(fd, buffer, offset, callback);
fs.read(fd, buffer, callback);
fs.read(fd, callback);

at least that is how i read the usage from the fs.write method that i thought was doing something similar:
https://github.com/nodejs/node/pull/31402/files#diff-9a205ef7ee967ee32efee02e58b3482dR547

If that is not the case, i'm happy to amend this to just do the signatures from the above post

Additionally, I think the language features used here may cause performance issues so it'd be better to just stick to the traditional type checking.

Which features? spread? performance isn't really in my wheel house, so i don't mind updating if that is the best way to go

Which features? spread? performance isn't really in my wheel house, so i don't mind updating if that is the best way to go

Spread in the parameter list may be ok, but last I checked destructuring and such caused performance regressions.

Spread in the parameter list may be ok, but last I checked destructuring and such caused performance regressions.

Might have been fixed in V8 7.8 (#30109).

I'm sort of drawing a blank on what other test should be added here. The only one I can think of, that wouldn't be duplicating something from an already existing test, is testing that the default buffer that is created is the correct size. I've added that one in the latest commit

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/513/

For API symmetry, this should be done also on FileHandle.prototype.read() in the fs.promises API (https://nodejs.org/dist/latest-v13.x/docs/api/fs.html#fs_filehandle_read_buffer_offset_length_position)

For API symmetry, this should be done also on FileHandle.prototype.read() in the fs.promises API

Yea, definitely. I'll probably send this in a second PR just to keep things clean

For API symmetry, this should be done also on FileHandle.prototype.read() in the fs.promises API

Yea, definitely. I'll probably send this in a second PR just to keep things clean

I just realized that these are just doc updates, so nvm, i can add to this pr 🤦‍♂️

Benchmark results:

                                                                                 confidence improvement accuracy (*)    (**)   (***)
 fs/readfile.js concurrent=10 len=1024 dur=5                                                     -0.35 %       ±5.12%  ±6.81%  ±8.87%
 fs/readfile.js concurrent=10 len=16777216 dur=5                                                 -0.51 %       ±3.46%  ±4.60%  ±5.99%
 fs/readfile.js concurrent=1 len=1024 dur=5                                                      -2.40 %       ±3.09%  ±4.12%  ±5.36%
 fs/readfile.js concurrent=1 len=16777216 dur=5                                                  -9.42 %      ±16.17% ±21.51% ±28.00%
 fs/readfile-partitioned.js concurrent=10 len=1024 dur=5                                         -1.92 %       ±5.05%  ±6.73%  ±8.79%
 fs/readfile-partitioned.js concurrent=10 len=16777216 dur=5                                     -2.00 %       ±4.07%  ±5.41%  ±7.04%
 fs/readfile-partitioned.js concurrent=1 len=1024 dur=5                                           2.70 %       ±6.40%  ±8.51% ±11.08%
 fs/readfile-partitioned.js concurrent=1 len=16777216 dur=5                                       0.15 %       ±3.23%  ±4.30%  ±5.61%
 fs/readFileSync.js n=600000                                                                     -0.63 %       ±8.40% ±11.20% ±14.63%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='asc'                    2.04 %       ±4.35%  ±5.79%  ±7.54%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='buf'             *      7.80 %       ±6.13%  ±8.17% ±10.65%
 fs/read-stream-throughput.js size=1024 filesize=1048576000 encodingType='utf'                    0.34 %       ±3.51%  ±4.67%  ±6.07%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='asc'         **      8.79 %       ±6.30%  ±8.47% ±11.20%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='buf'                -4.63 %      ±10.22% ±13.62% ±17.77%
 fs/read-stream-throughput.js size=1048576 filesize=1048576000 encodingType='utf'                 6.04 %      ±19.15% ±25.48% ±33.17%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='asc'                    1.61 %       ±5.53%  ±7.35%  ±9.57%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='buf'                    1.86 %       ±4.46%  ±5.93%  ±7.72%
 fs/read-stream-throughput.js size=4096 filesize=1048576000 encodingType='utf'             *     -6.08 %       ±4.60%  ±6.14%  ±8.02%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='asc'                  -3.43 %      ±10.17% ±13.53% ±17.62%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='buf'                   3.28 %       ±9.10% ±12.11% ±15.76%
 fs/read-stream-throughput.js size=65535 filesize=1048576000 encodingType='utf'                  -6.38 %       ±7.26%  ±9.69% ±12.66%

@jasnell @ronag just a friendly ping, i think my last comment may have gotten lost in the shuffle but does what i said here, #31402 (comment) make sense?

Hey sorry, I missed the earlier notification. I'd really prefer if the changes to fs.promise were in a separate commit in the same PR. Those apis should remain consistent and having it in a separate pr could make things more difficult

I'd really prefer if the changes to fs.promise were in a separate commit in the same PR. Those apis should remain consistent and having it in a separate pr could make things more difficult

Yea, i realized after i wrote that, it was just doc updates.

I guess my main question was about my thought process here:

I think i see what you are saying. Just for my own clarification, we keep the current signature of fs.read(fd, buffer, offset, lenght, position, callback), but then add another signature of fs.read(fd, options, callback) where the options is the buffer, offset, length, position params

So then if a user only wanted to specify some of the params, they would have to use the "options" object signature.

Does that sound correct?

but then add another signature of fs.read(fd, options, callback)

Ah, right... yes, that's exactly what I'm suggesting. It gives significantly more flexibility in how those various other properties are handled.

I've added a new commit that adds the signature fs.read(fs, options, callback) . just wanted to get some feedback first. I'm thinking the options object can also be optional, but i haven't coded that part yet

I added the optional options um...... option, so fs.read(fd, callback) will now work,

I think
test-fs-read-optional-options-params.js
test/parallel/test-fs-read-optional-params.js
can be just one file

Yea, probably. when i was writing those, i had them in the same file, but i was having an issue because i was re-using the same file. i'll try to rework it to make it work

@ronag I added another test and combined the 2 tests files into 1

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

Landed in b6da55f