From 95f03ef4201a3c3d7558703a488c108e965d0784 Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Thu, 16 Jan 2020 09:46:28 -0500 Subject: [PATCH 01/17] feat: make fs.read params optional 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 --- doc/api/fs.md | 6 +++++- lib/fs.js | 15 ++++++++++++++- test/parallel/test-fs-read.js | 2 +- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 736237f1f905b4..688d960fcec162 100644 --- a/doc/api/fs.md +++ b/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)` * `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} diff --git a/lib/fs.js b/lib/fs.js index 5916bc429bfe51..82f7c38d7f9bf6 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -455,7 +455,7 @@ function openSync(path, flags, mode) { } // usage: -// fs.read(fd, [buffer, offset[, length[, position]]], callback); +// fs.read(fd, [buffer[, offset[, length[, position]]], callback); function read(fd, ...args) { let callback = args.pop(); // Not really thrilled with this, but done to satisfy the linter From 10d3902db2ef8d089e25576bc794abaa8ee767a1 Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Wed, 22 Jan 2020 14:44:23 -0500 Subject: [PATCH 03/17] squash: adding in test --- test/parallel/test-fs-read-optional-params.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) create mode 100644 test/parallel/test-fs-read-optional-params.js diff --git a/test/parallel/test-fs-read-optional-params.js b/test/parallel/test-fs-read-optional-params.js new file mode 100644 index 00000000000000..d60d380e735a86 --- /dev/null +++ b/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); +})); From dfdd3b42693579b4b27c48eaf598b320735ec332 Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Wed, 22 Jan 2020 15:19:01 -0500 Subject: [PATCH 04/17] squash: better use of destructoring --- lib/fs.js | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 82f7c38d7f9bf6..174162f8f2b9c1 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -458,12 +458,9 @@ function openSync(path, flags, mode) { // fs.read(fd, [buffer[, offset[, length[, position]]], callback); function read(fd, ...args) { let callback = args.pop(); - // Not really thrilled with this, but done to satisfy the linter - // Maybe adding a linter exception here might be better? - const [buffer = Buffer.alloc(16384)] = args; - args.shift(); - let [ + const [buffer = Buffer.alloc(16384)] = args; + let [, offset = 0, length = buffer.length, position = null ] = args; From cf320a00a57339e40d6fa4e9fa81298c2e3c7d18 Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Tue, 11 Feb 2020 15:15:48 -0500 Subject: [PATCH 05/17] squash: refactor. now using the options object instead of making all things optional --- doc/api/fs.md | 31 +++++++++++++++++++ lib/fs.js | 27 ++++++++++------ test/parallel/test-fs-read-optional-params.js | 2 +- test/parallel/test-fs-read.js | 2 +- 4 files changed, 51 insertions(+), 11 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index b031fa6e895467..fc94e9c5f56526 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2764,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)` + +* `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` +* `callback` {Function} + * `err` {Error} + * `bytesRead` {integer} + * `buffer` {Buffer} + ## `fs.readdir(path[, options], callback)` +* `options` {Object} + * `buffer` {Buffer|Uint8Array} + * `offset` {integer} + * `length` {integer} + * `position` {integer} +* Returns: {Promise} + #### `filehandle.readFile(options)` From 128aeaba03fa3611466e2446d62e2da05c72f7a3 Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Wed, 12 Feb 2020 11:48:52 -0500 Subject: [PATCH 08/17] squash: reverting some doc updates --- doc/api/fs.md | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 0cbb1aeaf5bb11..b140a6cdd60104 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2716,14 +2716,10 @@ 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)` * `fd` {integer} -* `buffer` {Buffer|TypedArray|DataView} **Default:** `Buffer.alloc(16384)` -* `offset` {integer} **Default:** `0` -* `length` {integer} **Default:** `buffer.length` -* `position` {integer} **Default:** `null` +* `buffer` {Buffer|TypedArray|DataView} +* `offset` {integer} +* `length` {integer} +* `position` {integer} * `callback` {Function} * `err` {Error} * `bytesRead` {integer} From 2c5d3e7dbc18f1c475949502f34495aa02ad0e90 Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Thu, 13 Feb 2020 09:22:11 -0500 Subject: [PATCH 09/17] squash: Update doc/api/fs.md Co-Authored-By: James M Snell --- doc/api/fs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index b140a6cdd60104..6de708cdcdb100 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2762,7 +2762,7 @@ a `Promise` for an `Object` with `bytesRead` and `buffer` properties. ## `fs.read(fd, options, callback)` From 39239751e2b0348685fd6df37bb6879eecbb862a Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Thu, 13 Feb 2020 15:45:44 -0500 Subject: [PATCH 11/17] squash: add test for promise based --- test/parallel/test-fs-read-optional-params.js | 2 -- .../test-fs-read-promises-optional-params.js | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 2 deletions(-) create mode 100644 test/parallel/test-fs-read-promises-optional-params.js diff --git a/test/parallel/test-fs-read-optional-params.js b/test/parallel/test-fs-read-optional-params.js index ff8fb2fde7d526..bae5b74bfa0d2b 100644 --- a/test/parallel/test-fs-read-optional-params.js +++ b/test/parallel/test-fs-read-optional-params.js @@ -10,8 +10,6 @@ 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); diff --git a/test/parallel/test-fs-read-promises-optional-params.js b/test/parallel/test-fs-read-promises-optional-params.js new file mode 100644 index 00000000000000..9d19eaa41151a8 --- /dev/null +++ b/test/parallel/test-fs-read-promises-optional-params.js @@ -0,0 +1,19 @@ +'use strict'; + +const common = require('../common'); +const fixtures = require('../common/fixtures'); +const fs = require('fs'); +const read = require('util').promisify(fs.read); +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); + +read(fd, {}) + .then(function({ bytesRead, buffer }) { + assert.strictEqual(bytesRead, expected.length); + assert.deepStrictEqual(defaultBufferAsync.length, buffer.length); + }) + .then(common.mustCall()); From f6f01b65a4d0d22f0ce037ebce9e8b474915dc1c Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Fri, 14 Feb 2020 11:10:53 -0500 Subject: [PATCH 12/17] squash: add defaults to filehandler.read docs --- doc/api/fs.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index d2d6c6d055607f..483e395d071e99 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -4367,10 +4367,10 @@ property that is a reference to the passed in `buffer` argument. added: REPLACEME --> * `options` {Object} - * `buffer` {Buffer|Uint8Array} - * `offset` {integer} - * `length` {integer} - * `position` {integer} + * `buffer` {Buffer|Uint8Array} **Default:** `Buffer.alloc(16384)` + * `offset` {integer} **Default:** `0` + * `length` {integer} **Default:** `buffer.length` + * `position` {integer} **Default:** `null` * Returns: {Promise} #### `filehandle.readFile(options)` From 55a23561da846d839255faccc5cc85b1bfb9dc85 Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Wed, 26 Feb 2020 12:14:52 -0500 Subject: [PATCH 13/17] squash: add the use case for an optional options object --- lib/fs.js | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 1ee03e22503825..8f22db6b9482ca 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -462,12 +462,19 @@ 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; + // Assume fs.read(fd, options, callback) + let options = {}; + if (arguments.length < 3) { + // This is fs.read(fd, callback) + // buffer will be the callback + callback = buffer; + } else { + // This is fs.read(fd, {}, callback) + // buffer will be the options object + // offset is the callback + options = buffer; + callback = offset; + } buffer = options.buffer || Buffer.alloc(16384); offset = options.offset || 0; From f86bdc1e7cf476aba6280ba3615e03d4a6f1fd53 Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Wed, 26 Feb 2020 12:26:14 -0500 Subject: [PATCH 14/17] squash: add test for optional options option --- .../test-fs-read-optional-options-params.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) create mode 100644 test/parallel/test-fs-read-optional-options-params.js diff --git a/test/parallel/test-fs-read-optional-options-params.js b/test/parallel/test-fs-read-optional-options-params.js new file mode 100644 index 00000000000000..9a4206143be84b --- /dev/null +++ b/test/parallel/test-fs-read-optional-options-params.js @@ -0,0 +1,16 @@ +'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); + +fs.read(fd, common.mustCall((err, bytesRead, buffer) => { + assert.strictEqual(bytesRead, expected.length); + assert.deepStrictEqual(defaultBufferAsync.length, buffer.length); +})); From a4c3763f2a508981551e3c3b8522c14fb21f35cc Mon Sep 17 00:00:00 2001 From: Lucas Holmquist Date: Wed, 26 Feb 2020 12:33:15 -0500 Subject: [PATCH 15/17] squash: a little more doc update --- doc/api/fs.md | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 483e395d071e99..d32120fa5650cb 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2760,14 +2760,14 @@ 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)` +## `fs.read(fd, [options,] callback)` * `fd` {integer} * `options` {Object} @@ -2780,6 +2780,11 @@ changes: * `bytesRead` {integer} * `buffer` {Buffer} +Similar to the above `fs.read` function, this version takes an optional `options` object. If no `options` object +is specified, it will default with the above values. + + + ## `fs.readdir(path[, options], callback)`