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.

Sign up for GitHub

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

Jump to bottom

feat: make fs.read params optional #31402

Closed
wants to merge 17 commits into from
Closed

feat: make fs.read params optional #31402

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
95f03ef
feat: make fs.read params optional
lholmquist Jan 16, 2020
478d2f0
squash: doc updates
lholmquist Jan 22, 2020
10d3902
squash: adding in test
lholmquist Jan 22, 2020
dfdd3b4
squash: better use of destructoring
lholmquist Jan 22, 2020
cf320a0
squash: refactor.
lholmquist Feb 11, 2020
418b6fd
squash: Update doc/api/fs.md
lholmquist Feb 12, 2020
e3835c2
squash: Update doc/api/fs.md
lholmquist Feb 12, 2020
128aeab
squash: reverting some doc updates
lholmquist Feb 12, 2020
2c5d3e7
squash: Update doc/api/fs.md
lholmquist Feb 13, 2020
6433727
squash: Update doc/api/fs.md
lholmquist Feb 13, 2020
3923975
squash: add test for promise based
lholmquist Feb 13, 2020
f6f01b6
squash: add defaults to filehandler.read docs
lholmquist Feb 14, 2020
55a2356
squash: add the use case for an optional options object
lholmquist Feb 26, 2020
f86bdc1
squash: add test for optional options option
lholmquist Feb 26, 2020
a4c3763
squash: a little more doc update
lholmquist Feb 26, 2020
e321b88
squash: doc linting
lholmquist Feb 26, 2020
cb8c0ef
squash: combine tests and add another test that passes options
lholmquist Mar 6, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions doc/api/fs.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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

@lholmquist lholmquist Jan 28, 2020 •
edited
Loading

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 Down
12 changes: 11 additions & 1 deletion lib/fs.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -454,7 +454,17 @@ function openSync(path, flags, mode) {
return result;
}

function read(fd, buffer, offset, length, position, callback) {
// usage:
// fs.read(fd, [buffer[, offset[, length[, position]]], callback);
function read(fd, ...args) {
let callback = args.pop();
ronag marked this conversation as resolved.
Show resolved Hide resolved

const [buffer = Buffer.alloc(16384)] = args;
let [,
offset = 0,
length = buffer.length,
position = null ] = args;

validateInt32(fd, 'fd', 0);
validateBuffer(buffer);
callback = maybeCallback(callback);
Expand Down
18 changes: 18 additions & 0 deletions test/parallel/test-fs-read-optional-params.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
2 changes: 1 addition & 1 deletion test/parallel/test-fs-read.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ assert.throws(() => new fs.Dir(), {
assert.throws(
() => fs.read(fd, Buffer.alloc(1), 0, 1, 0),
{
message: 'Callback must be a function. Received undefined',
message: 'Callback must be a function. Received 0',
code: 'ERR_INVALID_CALLBACK',
}
);
Expand Down