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