feat: make fs.read params optional #31402

lholmquist · 2020-01-17T19:58:09Z

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 makes all the parameters of the fs.read function, except
for fd and the callback(when not using as a promise) optional.

fixes #31237

I still need to add a few tests(i've done some manual testing), but wanted to get this out to start the discussion.

There is one part, which i've commented in the code, where i did something to satisfy the linting, but i wasn't thrilled with it. Maybe having a lint exception here might be better? Looking for input on that

I'm not a typescript user, but i wonder if changing the function signature will somehow have a negative effect

This makes all the parameters of the `fs.read` function, except for `fd` and the callback(when not using as a promise) optional. They will default to sensible defaults fixes nodejs#31237

mscdex · 2020-01-17T21:15:21Z

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.

lholmquist · 2020-01-18T02:26:57Z

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

mscdex · 2020-01-18T05:16:07Z

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.

ronag · 2020-01-18T11:42:08Z

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).

lib/fs.js

lholmquist · 2020-01-22T19:44:59Z

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

ronag · 2020-01-27T20:49:41Z

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

jasnell · 2020-01-27T21:44:16Z

doc/api/fs.md

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

-## `fs.read(fd, buffer, offset, length, position, callback)`
+## `fs.read(fd, [buffer[, [offset[, length[, position]]]], callback)`


As much as I dislike polymorphic signatures, I'd much prefer the approach of moving to an options object as an alternative here. That is:

fs.read(fd, { offset: n, position: n }, callback)

It accomplishes the same goal with a much cleaner API and implementation, without the ambiguity of which argument was meant to be passed in.

@jasnell 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?

One thing that jumps out at me here, is when checking to see if that second parameter is the options object instead of the buffer object, i don't think we can use typeof since both would return object. Or am i overthinking this?

@jasnell Something else that i thought of was that the fs.write method is similar, in that it allows for optional parameters to be passed without the use of an options object.

Perhaps we don't go the options object route? Or if we do, maybe that function and others(not sure if there are)should be updated also?

I think moving that direction is ideal but doesn't have to be done all at once

@jasnell @ronag does what i said here, #31402 (comment) makes sense?

jasnell · 2020-01-27T21:46:11Z

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)

lholmquist · 2020-01-28T13:48:14Z

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

lholmquist · 2020-01-28T19:30:11Z

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 🤦‍♂️

ronag · 2020-01-28T20:23:28Z

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%

lholmquist · 2020-02-04T18:36:17Z

@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?

jasnell · 2020-02-04T18:40:46Z

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

lholmquist · 2020-02-04T18:47:20Z

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?

jasnell · 2020-02-04T18:49:15Z

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.

now using the options object instead of making all things optional

lholmquist · 2020-02-11T20:17:14Z

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

doc/api/fs.md

Co-Authored-By: Richard Lau <riclau@uk.ibm.com>

lholmquist · 2020-02-26T17:33:45Z

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

doc/api/fs.md

test/parallel/test-fs-read-optional-params.js

ronag

I think

test-fs-read-optional-options-params.js
test/parallel/test-fs-read-optional-params.js

can be just one file

lholmquist · 2020-03-03T13:44:39Z

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

lholmquist · 2020-03-06T14:52:16Z

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

ronag

LGTM

nodejs-github-bot · 2020-03-10T19:22:48Z

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

This makes all the parameters of the `fs.read` function, except for `fd` and the callback(when not using as a promise) optional. They will default to sensible defaults. Fixes: #31237 PR-URL: #31402 Reviewed-By: Robert Nagy <ronagy@icloud.com>

addaleax · 2020-03-11T15:49:03Z

Landed in b6da55f

This makes all the parameters of the `fs.read` function, except for `fd` and the callback(when not using as a promise) optional. They will default to sensible defaults. Fixes: #31237 PR-URL: #31402 Reviewed-By: Robert Nagy <ronagy@icloud.com>

himself65 · 2020-03-25T08:29:32Z

doc/api/fs.md

+  * `buffer` {Buffer|Uint8Array} **Default:** `Buffer.alloc(16384)`
+  * `offset` {integer} **Default:** `0`
+  * `length` {integer} **Default:** `buffer.length`
+  * `position` {integer} **Default:** `null`


This is not the same as the actual. options.position is the undefined when default

himself65 · 2020-03-25T08:29:40Z

doc/api/fs.md

+  * `buffer` {Buffer|TypedArray|DataView} **Default:** `Buffer.alloc(16384)`
+  * `offset` {integer} **Default:** `0`
+  * `length` {integer} **Default:** `buffer.length`
+  * `position` {integer} **Default:** `null`


This makes all the parameters of the `fs.read` function, except for `fd` and the callback(when not using as a promise) optional. They will default to sensible defaults. Fixes: nodejs#31237 PR-URL: nodejs#31402 Reviewed-By: Robert Nagy <ronagy@icloud.com>

This makes all the parameters of the `fs.read` function, except for `fd` and the callback(when not using as a promise) optional. They will default to sensible defaults. Fixes: #31237 PR-URL: #31402 Reviewed-By: Robert Nagy <ronagy@icloud.com>

feat: make fs.read params optional

95f03ef

This makes all the parameters of the `fs.read` function, except for `fd` and the callback(when not using as a promise) optional. They will default to sensible defaults fixes nodejs#31237

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

BridgeAR added the wip Issues and PRs that are still a work in progress. label Jan 20, 2020

squash: doc updates

478d2f0

ronag reviewed Jan 22, 2020

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

ronag reviewed Jan 22, 2020

View reviewed changes

lib/fs.js Outdated Show resolved Hide resolved

squash: adding in test

10d3902

squash: better use of destructoring

dfdd3b4

ronag removed the wip Issues and PRs that are still a work in progress. label Jan 27, 2020

jasnell reviewed Jan 27, 2020

View reviewed changes

squash: refactor.

cf320a0

now using the options object instead of making all things optional

richardlau reviewed Feb 12, 2020

View reviewed changes

doc/api/fs.md Outdated Show resolved Hide resolved

doc/api/fs.md Outdated Show resolved Hide resolved

lholmquist and others added 3 commits February 12, 2020 08:04

squash: Update doc/api/fs.md

418b6fd

Co-Authored-By: Richard Lau <riclau@uk.ibm.com>

squash: Update doc/api/fs.md

e3835c2

Co-Authored-By: Richard Lau <riclau@uk.ibm.com>

squash: reverting some doc updates

128aeab

lholmquist added 2 commits February 26, 2020 12:26

squash: add test for optional options option

f86bdc1

squash: a little more doc update

a4c3763

squash: doc linting

e321b88

ronag reviewed Feb 28, 2020

View reviewed changes

doc/api/fs.md Show resolved Hide resolved

ronag approved these changes Feb 28, 2020

View reviewed changes

ronag reviewed Feb 28, 2020

View reviewed changes

test/parallel/test-fs-read-optional-params.js Show resolved Hide resolved

ronag requested changes Feb 28, 2020

View reviewed changes

squash: combine tests and add another test that passes options

cb8c0ef

lholmquist requested a review from ronag March 9, 2020 14:32

ronag approved these changes Mar 10, 2020

View reviewed changes

ronag added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Mar 10, 2020

addaleax added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 11, 2020

addaleax approved these changes Mar 11, 2020

View reviewed changes

addaleax closed this Mar 11, 2020

MylesBorins mentioned this pull request Mar 12, 2020

v13.11.0 proposal #32185

Merged

lholmquist mentioned this pull request Mar 24, 2020

feat(fs): Make parameters optional for readSync #32460

Closed

4 tasks

himself65 mentioned this pull request Mar 24, 2020

test: fix a typo on test-fs-read-optional-params #32461

Closed

3 tasks

himself65 reviewed Mar 25, 2020

View reviewed changes

targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 25, 2020

targos mentioned this pull request May 2, 2020

v12.17.0 release proposal #33197

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: make fs.read params optional #31402

feat: make fs.read params optional #31402

lholmquist commented Jan 17, 2020 •

edited

Loading

mscdex commented Jan 17, 2020

lholmquist commented Jan 18, 2020

mscdex commented Jan 18, 2020

ronag commented Jan 18, 2020 •

edited

Loading

lholmquist commented Jan 22, 2020

ronag commented Jan 27, 2020

jasnell Jan 27, 2020

lholmquist Jan 28, 2020 •

edited

Loading

lholmquist Jan 28, 2020

jasnell Jan 28, 2020

lholmquist Jan 28, 2020

jasnell commented Jan 27, 2020

lholmquist commented Jan 28, 2020 •

edited

Loading

lholmquist commented Jan 28, 2020 •

edited

Loading

ronag commented Jan 28, 2020 •

edited

Loading

lholmquist commented Feb 4, 2020 •

edited

Loading

jasnell commented Feb 4, 2020

lholmquist commented Feb 4, 2020

jasnell commented Feb 4, 2020

lholmquist commented Feb 11, 2020

lholmquist commented Feb 26, 2020

ronag left a comment

lholmquist commented Mar 3, 2020

lholmquist commented Mar 6, 2020

ronag left a comment

nodejs-github-bot commented Mar 10, 2020

addaleax commented Mar 11, 2020

himself65 Mar 25, 2020

himself65 Mar 25, 2020

feat: make fs.read params optional #31402

feat: make fs.read params optional #31402

Conversation

lholmquist commented Jan 17, 2020 • edited Loading

Checklist

mscdex commented Jan 17, 2020

lholmquist commented Jan 18, 2020

mscdex commented Jan 18, 2020

ronag commented Jan 18, 2020 • edited Loading

lholmquist commented Jan 22, 2020

ronag commented Jan 27, 2020

jasnell Jan 27, 2020

Choose a reason for hiding this comment

lholmquist Jan 28, 2020 • edited Loading

Choose a reason for hiding this comment

lholmquist Jan 28, 2020

Choose a reason for hiding this comment

jasnell Jan 28, 2020

Choose a reason for hiding this comment

lholmquist Jan 28, 2020

Choose a reason for hiding this comment

jasnell commented Jan 27, 2020

lholmquist commented Jan 28, 2020 • edited Loading

lholmquist commented Jan 28, 2020 • edited Loading

ronag commented Jan 28, 2020 • edited Loading

lholmquist commented Feb 4, 2020 • edited Loading

jasnell commented Feb 4, 2020

lholmquist commented Feb 4, 2020

jasnell commented Feb 4, 2020

lholmquist commented Feb 11, 2020

lholmquist commented Feb 26, 2020

ronag left a comment

Choose a reason for hiding this comment

lholmquist commented Mar 3, 2020

lholmquist commented Mar 6, 2020

ronag left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Mar 10, 2020

addaleax commented Mar 11, 2020

himself65 Mar 25, 2020

Choose a reason for hiding this comment

himself65 Mar 25, 2020

Choose a reason for hiding this comment

lholmquist commented Jan 17, 2020 •

edited

Loading

ronag commented Jan 18, 2020 •

edited

Loading

lholmquist Jan 28, 2020 •

edited

Loading

lholmquist commented Jan 28, 2020 •

edited

Loading

lholmquist commented Jan 28, 2020 •

edited

Loading

ronag commented Jan 28, 2020 •

edited

Loading

lholmquist commented Feb 4, 2020 •

edited

Loading