Skip to content

Commit 9359de9

Browse files
committed
fs: make callback mandatory to all async functions
The "fs" module has two functions called `maybeCallback` and `makeCallback`, as of now. The `maybeCallback` creates a default function to report errors, if the parameter passed is not a function object. Basically, if the callback is omitted in some cases, this function is used to create a default callback function. The `makeCallback`, OTOH, creates a default function only if the parameter passed is `undefined`, and if it is not a function object it will throw an `Error`. This patch removes the `maybeCallback` function and makes the callback function argument mandatory for all the async functions. PR-URL: #7168 Reviewed-By: Trevor Norris <trev.norris@gmail.com>
1 parent 9983af0 commit 9359de9

10 files changed

+73
-143
lines changed

lib/fs.js

+49-93
Large diffs are not rendered by default.

test/fixtures/test-fs-readfile-error.js

-1
This file was deleted.

test/parallel/test-fs-chmod.js

+1-1
Original file line numberDiff line numberDiff line change
@@ -101,7 +101,7 @@ fs.open(file2, 'a', function(err, fd) {
101101
assert.equal(mode_sync, fs.fstatSync(fd).mode & 0o777);
102102
}
103103
success_count++;
104-
fs.close(fd);
104+
fs.closeSync(fd);
105105
}
106106
});
107107
});

test/parallel/test-fs-link.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -23,14 +23,14 @@ fs.link(srcPath, dstPath, common.mustCall(callback));
2323

2424
assert.throws(
2525
function() {
26-
fs.link();
26+
fs.link(undefined, undefined, () => {});
2727
},
2828
/src must be a string or Buffer/
2929
);
3030

3131
assert.throws(
3232
function() {
33-
fs.link('abc');
33+
fs.link('abc', undefined, () => {});
3434
},
3535
/dest must be a string or Buffer/
3636
);

test/parallel/test-fs-make-callback.js

-3
Original file line numberDiff line numberDiff line change
@@ -13,9 +13,6 @@ function test(cb) {
1313
// Verify the case where a callback function is provided
1414
assert.doesNotThrow(test(function() {}));
1515

16-
// Passing undefined calls rethrow() internally, which is fine
17-
assert.doesNotThrow(test(undefined));
18-
1916
// Anything else should throw
2017
assert.throws(test(null));
2118
assert.throws(test(true));

test/parallel/test-fs-mkdtemp.js

-5
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,3 @@ fs.mkdtemp(path.join(common.tmpDir, 'bar.'), common.mustCall(handler));
2929
// Same test as above, but making sure that passing an options object doesn't
3030
// affect the way the callback function is handled.
3131
fs.mkdtemp(path.join(common.tmpDir, 'bar.'), {}, common.mustCall(handler));
32-
33-
// Making sure that not passing a callback doesn't crash, as a default function
34-
// is passed internally.
35-
assert.doesNotThrow(() => fs.mkdtemp(path.join(common.tmpDir, 'bar-')));
36-
assert.doesNotThrow(() => fs.mkdtemp(path.join(common.tmpDir, 'bar-'), {}));
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
'use strict';
2+
3+
require('../common');
4+
const fs = require('fs');
5+
const assert = require('assert');
6+
7+
Object.getOwnPropertyNames(fs).filter(
8+
(d) => !d.endsWith('Stream') && // ignore stream creation functions
9+
!d.endsWith('Sync') && // ignore synchronous functions
10+
typeof fs[d] === 'function' && // ignore anything other than functions
11+
d.indexOf('_') === -1 && // ignore internal use functions
12+
!/^[A-Z]/.test(d) && // ignore conventional constructors
13+
d !== 'watch' && // watch's callback is optional
14+
d !== 'unwatchFile' // unwatchFile's callback is optional
15+
).forEach(
16+
(d) => assert.throws(() => fs[d](), /"callback" argument must be a function/)
17+
);

test/parallel/test-fs-readfile-error.js

-34
This file was deleted.

test/parallel/test-fs-stat.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,7 @@ fs.open('.', 'r', undefined, common.mustCall(function(err, fd) {
2828
fs.fstat(fd, common.mustCall(function(err, stats) {
2929
assert.ifError(err);
3030
assert.ok(stats.mtime instanceof Date);
31-
fs.close(fd);
31+
fs.closeSync(fd);
3232
assert(this === global);
3333
}));
3434

@@ -47,7 +47,7 @@ fs.open('.', 'r', undefined, common.mustCall(function(err, fd) {
4747
console.dir(stats);
4848
assert.ok(stats.mtime instanceof Date);
4949
}
50-
fs.close(fd);
50+
fs.closeSync(fd);
5151
}));
5252

5353
console.log(`stating: ${__filename}`);

test/parallel/test-fs-watchfile.js

+2-2
Original file line numberDiff line numberDiff line change
@@ -8,11 +8,11 @@ const assert = require('assert');
88
// Basic usage tests.
99
assert.throws(function() {
1010
fs.watchFile('./some-file');
11-
}, /"watchFile\(\)" requires a listener function/);
11+
}, /"callback" argument must be a function/);
1212

1313
assert.throws(function() {
1414
fs.watchFile('./another-file', {}, 'bad listener');
15-
}, /"watchFile\(\)" requires a listener function/);
15+
}, /"callback" argument must be a function/);
1616

1717
assert.throws(function() {
1818
fs.watchFile(new Object(), function() {});

0 commit comments

Comments
 (0)