fs: filehandle read now accepts object as argument

[branisha]

Fixes: #34176
Refs: https://nodejs.org/api/fs.html#fs_filehandle_read_options

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• commit message follows commit guidelines

[himself65]

and we need to add a test case, like this

diff --git a/test/parallel/test-fs-promises-file-handle-read.js b/test/parallel/test-fs-promises-file-handle-read.js
index 5ba7fc63e3..c9fc38d489 100644
--- a/test/parallel/test-fs-promises-file-handle-read.js+++ b/test/parallel/test-fs-promises-file-handle-read.js@@ -13,7 +13,24 @@ const tmpdir = require('../common/tmpdir');
const assert = require('assert');
const tmpDir = tmpdir.path;
-tmpdir.refresh();+let useConf = false;+(async () => {+ for (const value of [false, true]) {+ tmpdir.refresh();+ useConf = value;++ await validateRead()+ .then(validateEmptyRead)+ .then(validateLargeRead)+ .then(common.mustCall());+ }+})();++function read(fileHandle, buffer, offset, length, position) {+ return useConf ?+ fileHandle.read({ buffer, offset, length, position }) :+ fileHandle.read(buffer, offset, length, position);+}
async function validateRead() {
const filePath = path.resolve(tmpDir, 'tmp-read-file.txt');
@@ -23,7 +40,7 @@ async function validateRead() {
const fd = fs.openSync(filePath, 'w+');
fs.writeSync(fd, buffer, 0, buffer.length);
fs.closeSync(fd);
- const readAsyncHandle = await fileHandle.read(Buffer.alloc(11), 0, 11, 0);+ const readAsyncHandle = await read(fileHandle, Buffer.alloc(11), 0, 11, 0);
assert.deepStrictEqual(buffer.length, readAsyncHandle.bytesRead);
assert.deepStrictEqual(buffer, readAsyncHandle.buffer);
@@ -38,7 +55,7 @@ async function validateEmptyRead() {
const fd = fs.openSync(filePath, 'w+');
fs.writeSync(fd, buffer, 0, buffer.length);
fs.closeSync(fd);
- const readAsyncHandle = await fileHandle.read(Buffer.alloc(11), 0, 11, 0);+ const readAsyncHandle = await read(fileHandle, Buffer.alloc(11), 0, 11, 0);
assert.deepStrictEqual(buffer.length, readAsyncHandle.bytesRead);
await fileHandle.close();
@@ -51,12 +68,7 @@ async function validateLargeRead() {
const filePath = fixtures.path('x.txt');
const fileHandle = await open(filePath, 'r');
const pos = 0xffffffff + 1; // max-uint32 + 1
- const readHandle = await fileHandle.read(Buffer.alloc(1), 0, 1, pos);+ const readHandle = await read(fileHandle, Buffer.alloc(1), 0, 1, pos);
assert.strictEqual(readHandle.bytesRead, 0);
}
--validateRead()- .then(validateEmptyRead)- .then(validateLargeRead)- .then(common.mustCall());

[jasnell]

I would leave this signature alone. It's subtle, and not likely to break anyone, but changing this does not a small impact on the API and it's just not really necessary.

[branisha]

Yeah, I agree, it does look more nicer and cleaner the way it originally was. I would rather manually parse parameters. Your thoughts @himself65, @mscdex?

[himself65]

Another implementation is to parse the parameters in this class function.
Whatever, I’m 👎 with the previous handle which uses filter

[himself65]

move this to here maybe better?

if(arguments.length<3){// Assume filehandle.read(options)({
buffer =Buffer.alloc(16384),
offset =0,
length =buffer.length,
position
}=buffer);}

[branisha]

Yup, I agree, looks good here, I just did this, waiting for build to finish. Also, I changed arguments.length < 3 to arguments.length < 2 because we don't get filehandle reference argument in this method.

[aduh95]

@branisha Can you rebase this to resolve the git conflits please?

[github-actions]

This issue/PR was marked as stalled, it will be automatically closed in 30 days. If it should remain open, please leave a comment explaining why it should remain open.

[branisha]

@branisha Can you rebase this to resolve the git conflits please?

I'll try to find some time in the next couple of days to rebase this and update

[branisha]

@aduh95@jasnell@himself65
Since I last worked on this, quite has changed in FS operations. I've tried to make it sensible, and I hope it's up to standard. If it's not, please let me know, and lay some wrath on me 😅.

[himself65]

still LGTM

[nodejs-github-bot]

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

[aduh95]

Landed in d05c271