Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: make fs.read params optional #31402

Closed
Closed
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
45 changes: 40 additions & 5 deletions doc/api/fs.md
Expand Up @@ -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)`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

<!-- YAML
added: v0.0.2
changes:
- version: REPLACEME
pr-url: REPLACEME
richardlau marked this conversation as resolved.
Show resolved Hide resolved
description: Buffer, offset, length and position parameters
are now optional
- version: v10.10.0
pr-url: https://github.com/nodejs/node/pull/22150
description: The `buffer` parameter can now be any `TypedArray`, or a
Expand All @@ -2733,10 +2737,10 @@ changes:
-->

* `fd` {integer}
* `buffer` {Buffer|TypedArray|DataView}
* `offset` {integer}
* `length` {integer}
* `position` {integer}
* `buffer` {Buffer|TypedArray|DataView} **Default:** `Buffer.alloc(16384)`
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.length`
* `position` {integer} **Default:** `null`
* `callback` {Function}
* `err` {Error}
* `bytesRead` {integer}
Expand All @@ -2760,6 +2764,26 @@ The callback is given the three arguments, `(err, bytesRead, buffer)`.
If this method is invoked as its [`util.promisify()`][]ed version, it returns
a `Promise` for an `Object` with `bytesRead` and `buffer` properties.

## `fs.read(fd, options, callback)`
<!-- YAML
added: v0.0.2
lholmquist marked this conversation as resolved.
Show resolved Hide resolved
changes:
- version: REPLACEME
pr-url: REPLACEME
richardlau marked this conversation as resolved.
Show resolved Hide resolved
description: Options object can be passed in
to make Buffer, offset, lenght and position optional
-->
* `fd` {integer}
* `options` {Object}
* `buffer` {Buffer|TypedArray|DataView} **Default:** `Buffer.alloc(16384)`
* `offset` {integer} **Default:** `0`
* `length` {integer} **Default:** `buffer.length`
* `position` {integer} **Default:** `null`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

* `callback` {Function}
* `err` {Error}
* `bytesRead` {integer}
* `buffer` {Buffer}

## `fs.readdir(path[, options], callback)`
<!-- YAML
added: v0.1.8
Expand Down Expand Up @@ -4342,6 +4366,17 @@ Following successful read, the `Promise` is resolved with an object with a
`bytesRead` property specifying the number of bytes read, and a `buffer`
property that is a reference to the passed in `buffer` argument.

#### `filehandle.read({buffer, offset, length, position})`
lholmquist marked this conversation as resolved.
Show resolved Hide resolved
<!-- YAML
added: REPLACEME
-->
* `options` {Object}
* `buffer` {Buffer|Uint8Array}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the defaults here are the same as the fs.read(fd, options, callback) version? If so, they should be documented here too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I might be missing it but is the Promise version tested?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think you are missing anything. i think i forgot to add a test for the promise based call

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume the defaults here are the same as the fs.read(fd, options, callback) version? If so, they should be documented here too.

Right, added them and a test for the promise in the latest 2 commits

Also, I noticed that the buffer types for the filehandle.read function are not the same as the regular fs.read function buffer types. That doesn't seem correct to me

* `offset` {integer}
* `length` {integer}
* `position` {integer}
* Returns: {Promise}
ronag marked this conversation as resolved.
Show resolved Hide resolved

#### `filehandle.readFile(options)`
<!-- YAML
added: v10.0.0
Expand Down
19 changes: 19 additions & 0 deletions lib/fs.js
Expand Up @@ -454,8 +454,27 @@ function openSync(path, flags, mode) {
return result;
}

// usage:
// fs.read(fd, buffer, offset, length, position, callback);
// OR
// fs.read(fd, {}, callback)
function read(fd, buffer, offset, length, position, callback) {
validateInt32(fd, 'fd', 0);

if (arguments.length <= 3) {
// This is the fs.read(fd, {}, callback) version
// fd is fd
// buffer will be the options object
// offset is the callback
const options = buffer;
callback = offset;

buffer = options.buffer || Buffer.alloc(16384);
offset = options.offset || 0;
length = options.length || buffer.length;
position = options.position || null;
}

validateBuffer(buffer);
callback = maybeCallback(callback);

Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-fs-read-optional-params.js
@@ -0,0 +1,18 @@
'use strict';

const common = require('../common');
const fixtures = require('../common/fixtures');
const fs = require('fs');
const assert = require('assert');
const filepath = fixtures.path('x.txt');
const fd = fs.openSync(filepath, 'r');

const expected = Buffer.from('xyz\n');
const defaultBufferAsync = Buffer.alloc(16384);

// Optional buffer, offset, length, position
// fs.read(fd, callback);
fs.read(fd, {}, common.mustCall((err, bytesRead, buffer) => {
assert.strictEqual(bytesRead, expected.length);
assert.deepStrictEqual(defaultBufferAsync.length, buffer.length);
}));
ronag marked this conversation as resolved.
Show resolved Hide resolved