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

fs: Fix default params for fs.write #7856

Closed
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
20 changes: 10 additions & 10 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -1577,13 +1577,13 @@ _Note: [`fs.watch()`][] is more efficient than `fs.watchFile` and
`fs.unwatchFile`. `fs.watch` should be used instead of `fs.watchFile` and
`fs.unwatchFile` when possible._

## fs.write(fd, buffer, offset, length[, position], callback)
## fs.write(fd, buffer[, offset[, length[, position]]], callback)
<!-- YAML
added: v0.0.2
-->

* `fd` {Integer}
* `buffer` {String | Buffer}
* `buffer` {Buffer}
Copy link
Contributor

Choose a reason for hiding this comment

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

From the doc changes, this appears to be semver major, in that it removes the fs.write(fd, string, offset, length, position, callback) as a supported use. Was that usage not implemented, so its aligning docs with actual implementation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it wasn't ever implemented. The code ends up either calling binding.writeBuffer or binding.writeString based on whether the second parameter is a buffer, and those functions aren't polymorphic.

* `offset` {Integer}
* `length` {Integer}
* `position` {Integer}
Expand All @@ -1608,19 +1608,19 @@ On Linux, positional writes don't work when the file is opened in append mode.
The kernel ignores the position argument and always appends the data to
the end of the file.

## fs.write(fd, data[, position[, encoding]], callback)
## fs.write(fd, string[, position[, encoding]], callback)
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this syntax correct? Is it really impossible to specify encoding unless you also specify position? So, to write using an encoding at the current fd position, I would have to do fs.write(fd, string, nil, encoding), specifying a position that has typeof != number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct! It does seem a bit convoluted, yes. This time around I only meant to fix the bugs and make sure the documentation is correct, though :)

<!-- YAML
added: v0.11.5
-->

* `fd` {Integer}
* `data` {String | Buffer}
* `string` {String}
* `position` {Integer}
* `encoding` {String}
* `callback` {Function}

Write `data` to the file specified by `fd`. If `data` is not a Buffer instance
then the value will be coerced to a string.
Write `string` to the file specified by `fd`. If `string` is not a string, then
the value will be coerced to one.

`position` refers to the offset from the beginning of the file where this data
should be written. If `typeof position !== 'number'` the data will be written at
Expand Down Expand Up @@ -1701,24 +1701,24 @@ added: v0.1.29

The synchronous version of [`fs.writeFile()`][]. Returns `undefined`.

## fs.writeSync(fd, buffer, offset, length[, position])
## fs.writeSync(fd, buffer[, offset[, length[, position]]])
<!-- YAML
added: v0.1.21
-->

* `fd` {Integer}
* `buffer` {String | Buffer}
* `buffer` {Buffer}
* `offset` {Integer}
* `length` {Integer}
* `position` {Integer}

## fs.writeSync(fd, data[, position[, encoding]])
## fs.writeSync(fd, string[, position[, encoding]])
<!-- YAML
added: v0.11.5
-->

* `fd` {Integer}
* `data` {String | Buffer}
* `string` {String}
* `position` {Integer}
* `encoding` {String}

Expand Down
20 changes: 14 additions & 6 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -738,7 +738,7 @@ fs.readSync = function(fd, buffer, offset, length, position) {
};

// usage:
// fs.write(fd, buffer, offset, length[, position], callback);
// fs.write(fd, buffer[, offset[, length[, position]]], callback);
// OR
// fs.write(fd, string[, position[, encoding]], callback);
fs.write = function(fd, buffer, offset, length, position, callback) {
Expand All @@ -751,12 +751,16 @@ fs.write = function(fd, buffer, offset, length, position, callback) {
req.oncomplete = wrapper;

if (buffer instanceof Buffer) {
// if no position is passed then assume null
if (typeof position === 'function') {
callback = position;
callback = maybeCallback(callback || position || length || offset);
if (typeof offset !== 'number') {
offset = 0;
}
if (typeof length !== 'number') {
length = buffer.length - offset;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

The comment at the top of the function does not match public documentation. Mind fixing that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure thing, fixed in 4062d4cf. I changed both the comment and the documentation to match the implementation.

if (typeof position !== 'number') {
position = null;
}
callback = maybeCallback(callback);
return binding.writeBuffer(fd, buffer, offset, length, position, req);
}

Expand All @@ -776,13 +780,17 @@ fs.write = function(fd, buffer, offset, length, position, callback) {
};

// usage:
// fs.writeSync(fd, buffer, offset, length[, position]);
// fs.writeSync(fd, buffer[, offset[, length[, position]]]);
// OR
// fs.writeSync(fd, string[, position[, encoding]]);
fs.writeSync = function(fd, buffer, offset, length, position) {
if (buffer instanceof Buffer) {
if (position === undefined)
position = null;
if (typeof offset !== 'number')
offset = 0;
if (typeof length !== 'number')
length = buffer.length - offset;
return binding.writeBuffer(fd, buffer, offset, length, position);
}
if (typeof buffer !== 'string')
Expand Down
120 changes: 99 additions & 21 deletions test/parallel/test-fs-write-buffer.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,29 +2,107 @@
const common = require('../common');
const assert = require('assert');
const path = require('path');
const Buffer = require('buffer').Buffer;
const fs = require('fs');
const filename = path.join(common.tmpDir, 'write.txt');
const expected = Buffer.from('hello');

common.refreshTmpDir();

fs.open(filename, 'w', 0o644, common.mustCall(function(err, fd) {
if (err) throw err;

fs.write(fd,
expected,
0,
expected.length,
null,
common.mustCall(function(err, written) {
if (err) throw err;

assert.equal(expected.length, written);
fs.closeSync(fd);

var found = fs.readFileSync(filename, 'utf8');
assert.deepStrictEqual(expected.toString(), found);
fs.unlinkSync(filename);
}));
}));
// fs.write with all parameters provided:
{
const filename = path.join(common.tmpDir, 'write1.txt');
fs.open(filename, 'w', 0o644, common.mustCall(function(err, fd) {
assert.ifError(err);

const cb = common.mustCall(function(err, written) {
assert.ifError(err);

assert.strictEqual(expected.length, written);
fs.closeSync(fd);

var found = fs.readFileSync(filename, 'utf8');
assert.deepStrictEqual(expected.toString(), found);
});

fs.write(fd, expected, 0, expected.length, null, cb);
}));
}

// fs.write with a buffer, without the length parameter:
{
const filename = path.join(common.tmpDir, 'write2.txt');
fs.open(filename, 'w', 0o644, common.mustCall(function(err, fd) {
assert.ifError(err);

const cb = common.mustCall(function(err, written) {
assert.ifError(err);

assert.strictEqual(2, written);
fs.closeSync(fd);

const found = fs.readFileSync(filename, 'utf8');
assert.deepStrictEqual('lo', found);
});

fs.write(fd, Buffer.from('hello'), 3, cb);
}));
}

// fs.write with a buffer, without the offset and length parameters:
{
const filename = path.join(common.tmpDir, 'write3.txt');
fs.open(filename, 'w', 0o644, common.mustCall(function(err, fd) {
assert.ifError(err);

const cb = common.mustCall(function(err, written) {
assert.ifError(err);

assert.strictEqual(expected.length, written);
fs.closeSync(fd);

const found = fs.readFileSync(filename, 'utf8');
assert.deepStrictEqual(expected.toString(), found);
});

fs.write(fd, expected, cb);
}));
}

// fs.write with the offset passed as undefined followed by the callback:
{
const filename = path.join(common.tmpDir, 'write4.txt');
fs.open(filename, 'w', 0o644, common.mustCall(function(err, fd) {
assert.ifError(err);

const cb = common.mustCall(function(err, written) {
assert.ifError(err);

assert.strictEqual(expected.length, written);
fs.closeSync(fd);

const found = fs.readFileSync(filename, 'utf8');
assert.deepStrictEqual(expected.toString(), found);
});

fs.write(fd, expected, undefined, cb);
}));
}

// fs.write with offset and length passed as undefined followed by the callback:
{
const filename = path.join(common.tmpDir, 'write5.txt');
fs.open(filename, 'w', 0o644, common.mustCall(function(err, fd) {
assert.ifError(err);

const cb = common.mustCall(function(err, written) {
assert.ifError(err);

assert.strictEqual(expected.length, written);
fs.closeSync(fd);

const found = fs.readFileSync(filename, 'utf8');
assert.deepStrictEqual(expected.toString(), found);
});

fs.write(fd, expected, undefined, undefined, cb);
}));
}
63 changes: 48 additions & 15 deletions test/parallel/test-fs-write-sync.js
Original file line number Diff line number Diff line change
@@ -1,23 +1,56 @@
'use strict';
var common = require('../common');
var assert = require('assert');
var path = require('path');
var fs = require('fs');
var fn = path.join(common.tmpDir, 'write.txt');
const common = require('../common');
const assert = require('assert');
const path = require('path');
const fs = require('fs');
const filename = path.join(common.tmpDir, 'write.txt');

common.refreshTmpDir();

var foo = 'foo';
var fd = fs.openSync(fn, 'w');
// fs.writeSync with all parameters provided:
{
const fd = fs.openSync(filename, 'w');

var written = fs.writeSync(fd, '');
assert.strictEqual(0, written);
let written = fs.writeSync(fd, '');
assert.strictEqual(0, written);

fs.writeSync(fd, foo);
fs.writeSync(fd, 'foo');

var bar = 'bár';
written = fs.writeSync(fd, Buffer.from(bar), 0, Buffer.byteLength(bar));
assert.ok(written > 3);
fs.closeSync(fd);
written = fs.writeSync(fd, Buffer.from('bár'), 0, Buffer.byteLength('bár'));
assert.ok(written > 3);
fs.closeSync(fd);

assert.equal(fs.readFileSync(fn), 'foobár');
assert.strictEqual(fs.readFileSync(filename, 'utf-8'), 'foobár');
}

// fs.writeSync with a buffer, without the length parameter:
{
const fd = fs.openSync(filename, 'w');

let written = fs.writeSync(fd, '');
assert.strictEqual(0, written);

fs.writeSync(fd, 'foo');

written = fs.writeSync(fd, Buffer.from('bár'), 0);
assert.ok(written > 3);
fs.closeSync(fd);

assert.strictEqual(fs.readFileSync(filename, 'utf-8'), 'foobár');
}

// fs.writeSync with a buffer, without the offset and length parameters:
{
const fd = fs.openSync(filename, 'w');

let written = fs.writeSync(fd, '');
assert.strictEqual(0, written);

fs.writeSync(fd, 'foo');

written = fs.writeSync(fd, Buffer.from('bár'));
assert.ok(written > 3);
fs.closeSync(fd);

assert.strictEqual(fs.readFileSync(filename, 'utf-8'), 'foobár');
}