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

Update fs promise shims to support latest Node functions #747

Merged
merged 4 commits into from
Feb 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,7 @@ Methods
- [writeJsonSync](docs/writeJson-sync.md)


**NOTE:** You can still use the native Node.js methods. They are promisified and copied over to `fs-extra`. See [notes on `fs.read()` & `fs.write()`](docs/fs-read-write.md)
**NOTE:** You can still use the native Node.js methods. They are promisified and copied over to `fs-extra`. See [notes on `fs.read()`, `fs.write()`, & `fs.writev()`](docs/fs-read-write-writev.md)

### What happened to `walk()` and `walkSync()`?

Expand Down
11 changes: 10 additions & 1 deletion docs/fs-read-write.md → docs/fs-read-write-writev.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# About `fs.read()` & `fs.write()`

[`fs.read()`](https://nodejs.org/api/fs.html#fs_fs_read_fd_buffer_offset_length_position_callback) & [`fs.write()`](https://nodejs.org/api/fs.html#fs_fs_write_fd_buffer_offset_length_position_callback) are different from other `fs` methods in that their callbacks are called with 3 arguments instead of the usual 2 arguments.
[`fs.read()`](https://nodejs.org/api/fs.html#fs_fs_read_fd_buffer_offset_length_position_callback), [`fs.write()`](https://nodejs.org/api/fs.html#fs_fs_write_fd_buffer_offset_length_position_callback), & [`fs.writev()`](https://nodejs.org/api/fs.html#fs_fs_writev_fd_buffers_position_callback) are different from other `fs` methods in that their callbacks are called with 3 arguments instead of the usual 2 arguments.

If you're using them with callbacks, they will behave as usual. However, their promise usage is a little different. `fs-extra` promisifies these methods like [`util.promisify()`](https://nodejs.org/api/util.html#util_util_promisify_original) (only available in Node 8+) does.

Expand Down Expand Up @@ -37,3 +37,12 @@ async function example () {
const { bytesWritten, buffer } = await fs.write(fd, Buffer.alloc(length), offset, length, position)
}
```

## `fs.writev()`

```js
// With async/await:
async function example () {
const { bytesWritten, buffers } = await fs.writev(fd, buffers, position)
}
```
58 changes: 58 additions & 0 deletions lib/fs/__tests__/multi-param.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ const SIZE = 1000

// Used for tests on Node 7.2.0+ only
const onNode7it = semver.gte(process.version, '7.2.0') ? it : it.skip
// Used for tests on Node 12.9.0+ only
const describeNode12 = semver.gte(process.version, '12.9.0') ? describe : describe.skip

describe('fs.read()', () => {
let TEST_FILE
Expand Down Expand Up @@ -152,3 +154,59 @@ describe('fs.write()', () => {
})
})
})

describeNode12('fs.writev()', () => {
let TEST_FILE
let TEST_DATA
let TEST_FD

beforeEach(() => {
TEST_FILE = path.join(os.tmpdir(), 'fs-extra', 'writev-test-file')
TEST_DATA = [crypto.randomBytes(SIZE / 2), crypto.randomBytes(SIZE / 2)]
fs.ensureDirSync(path.dirname(TEST_FILE))
TEST_FD = fs.openSync(TEST_FILE, 'w')
})

afterEach(() => {
return fs.close(TEST_FD)
.then(() => fs.remove(TEST_FILE))
})

describe('with promises', () => {
it('returns an object', () => {
return fs.writev(TEST_FD, TEST_DATA, 0)
.then(({ bytesWritten, buffers }) => {
assert.strictEqual(bytesWritten, SIZE, 'bytesWritten is correct')
Copy link
Collaborator

@manidlou manidlou Feb 1, 2020

Choose a reason for hiding this comment

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

I believe the message should be the opposite, something like bytesWritten is incorrect since that is used as the message for AssertionError if the values are not strictly equal. It is the same for other assertion messages in other tests down below.

https://nodejs.org/api/assert.html#assert_assert_strictequal_actual_expected_message

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, perhaps; I don't think it really matters, the main point is to clearly identify which assertion is throwing if you have an error. In any case, these are simply copied from tests earlier in the file, so if we want to fix this, it should be in a separate PR that fixes the entire file.

assert.deepStrictEqual(buffers, TEST_DATA, 'data is correct')
})
})

it('returns an object when minimal arguments are passed', () => {
return fs.writev(TEST_FD, TEST_DATA)
.then(({ bytesWritten, buffers }) => {
assert.strictEqual(bytesWritten, SIZE, 'bytesWritten is correct')
assert.deepStrictEqual(buffers, TEST_DATA, 'data is correct')
})
})
})

describe('with callbacks', () => {
it('works', done => {
fs.writev(TEST_FD, TEST_DATA, 0, (err, bytesWritten, buffers) => {
assert.ifError(err)
assert.strictEqual(bytesWritten, SIZE, 'bytesWritten is correct')
assert.deepStrictEqual(buffers, TEST_DATA, 'data is correct')
done()
})
})

it('works when minimal arguments are passed', done => {
fs.writev(TEST_FD, TEST_DATA, (err, bytesWritten, buffers) => {
assert.ifError(err)
assert.strictEqual(bytesWritten, SIZE, 'bytesWritten is correct')
assert.deepStrictEqual(buffers, TEST_DATA, 'data is correct')
done()
})
})
})
})
27 changes: 24 additions & 3 deletions lib/fs/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,15 +18,16 @@ const api = [
'fsync',
'ftruncate',
'futimes',
'lchown',
'lchmod',
'lchown',
'link',
'lstat',
'mkdir',
'mkdtemp',
'open',
'readFile',
'opendir',
'readdir',
'readFile',
'readlink',
'realpath',
'rename',
Expand All @@ -39,6 +40,7 @@ const api = [
'writeFile'
].filter(key => {
// Some commands are not available on some systems. Ex:
// fs.opendir was added in Node.js v12.12.0
// fs.copyFile was added in Node.js v8.5.0
// fs.mkdtemp was added in Node.js v5.10.0
// fs.lchown is not available on at least some Linux
Expand Down Expand Up @@ -71,7 +73,7 @@ exports.exists = function (filename, callback) {
})
}

// fs.read() & fs.write need special treatment due to multiple callback args
// fs.read(), fs.write(), & fs.writev() need special treatment due to multiple callback args

exports.read = function (fd, buffer, offset, length, position, callback) {
if (typeof callback === 'function') {
Expand Down Expand Up @@ -103,6 +105,25 @@ exports.write = function (fd, buffer, ...args) {
})
}

// fs.writev only available in Node v12.9.0+
if (typeof fs.writev === 'function') {
// Function signature is
// s.writev(fd, buffers[, position], callback)
// We need to handle the optional arg, so we use ...args
exports.writev = function (fd, buffers, ...args) {
if (typeof args[args.length - 1] === 'function') {
return fs.writev(fd, buffers, ...args)
}

return new Promise((resolve, reject) => {
fs.writev(fd, buffers, ...args, (err, bytesWritten, buffers) => {
if (err) return reject(err)
resolve({ bytesWritten, buffers })
})
})
}
}

// fs.realpath.native only available in Node v9.2+
if (typeof fs.realpath.native === 'function') {
exports.realpath.native = u(fs.realpath.native)
Expand Down