Skip to content

Commit

Permalink
fs: make callback mandatory to all async functions
Browse files Browse the repository at this point in the history
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: nodejs#7168
Reviewed-By: Trevor Norris <trev.norris@gmail.com>
  • Loading branch information
thefourtheye committed Jul 21, 2016
1 parent 9983af0 commit 9359de9
Show file tree
Hide file tree
Showing 10 changed files with 73 additions and 143 deletions.
142 changes: 49 additions & 93 deletions lib/fs.js

Large diffs are not rendered by default.

1 change: 0 additions & 1 deletion test/fixtures/test-fs-readfile-error.js

This file was deleted.

2 changes: 1 addition & 1 deletion test/parallel/test-fs-chmod.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ fs.open(file2, 'a', function(err, fd) {
assert.equal(mode_sync, fs.fstatSync(fd).mode & 0o777);
}
success_count++;
fs.close(fd);
fs.closeSync(fd);
}
});
});
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-fs-link.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,14 @@ fs.link(srcPath, dstPath, common.mustCall(callback));

assert.throws(
function() {
fs.link();
fs.link(undefined, undefined, () => {});
},
/src must be a string or Buffer/
);

assert.throws(
function() {
fs.link('abc');
fs.link('abc', undefined, () => {});
},
/dest must be a string or Buffer/
);
3 changes: 0 additions & 3 deletions test/parallel/test-fs-make-callback.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,6 @@ function test(cb) {
// Verify the case where a callback function is provided
assert.doesNotThrow(test(function() {}));

// Passing undefined calls rethrow() internally, which is fine
assert.doesNotThrow(test(undefined));

// Anything else should throw
assert.throws(test(null));
assert.throws(test(true));
Expand Down
5 changes: 0 additions & 5 deletions test/parallel/test-fs-mkdtemp.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,8 +29,3 @@ fs.mkdtemp(path.join(common.tmpDir, 'bar.'), common.mustCall(handler));
// Same test as above, but making sure that passing an options object doesn't
// affect the way the callback function is handled.
fs.mkdtemp(path.join(common.tmpDir, 'bar.'), {}, common.mustCall(handler));

// Making sure that not passing a callback doesn't crash, as a default function
// is passed internally.
assert.doesNotThrow(() => fs.mkdtemp(path.join(common.tmpDir, 'bar-')));
assert.doesNotThrow(() => fs.mkdtemp(path.join(common.tmpDir, 'bar-'), {}));
17 changes: 17 additions & 0 deletions test/parallel/test-fs-no-callback-errors.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
'use strict';

require('../common');
const fs = require('fs');
const assert = require('assert');

Object.getOwnPropertyNames(fs).filter(
(d) => !d.endsWith('Stream') && // ignore stream creation functions
!d.endsWith('Sync') && // ignore synchronous functions
typeof fs[d] === 'function' && // ignore anything other than functions
d.indexOf('_') === -1 && // ignore internal use functions
!/^[A-Z]/.test(d) && // ignore conventional constructors
d !== 'watch' && // watch's callback is optional
d !== 'unwatchFile' // unwatchFile's callback is optional
).forEach(
(d) => assert.throws(() => fs[d](), /"callback" argument must be a function/)
);
34 changes: 0 additions & 34 deletions test/parallel/test-fs-readfile-error.js

This file was deleted.

4 changes: 2 additions & 2 deletions test/parallel/test-fs-stat.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ fs.open('.', 'r', undefined, common.mustCall(function(err, fd) {
fs.fstat(fd, common.mustCall(function(err, stats) {
assert.ifError(err);
assert.ok(stats.mtime instanceof Date);
fs.close(fd);
fs.closeSync(fd);
assert(this === global);
}));

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

console.log(`stating: ${__filename}`);
Expand Down
4 changes: 2 additions & 2 deletions test/parallel/test-fs-watchfile.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,11 @@ const assert = require('assert');
// Basic usage tests.
assert.throws(function() {
fs.watchFile('./some-file');
}, /"watchFile\(\)" requires a listener function/);
}, /"callback" argument must be a function/);

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

assert.throws(function() {
fs.watchFile(new Object(), function() {});
Expand Down

0 comments on commit 9359de9

Please sign in to comment.