Skip to content

Commit

Permalink
Revert "fs: make callback mandatory to all async functions"
Browse files Browse the repository at this point in the history
This reverts commit 9359de9.

Original Commit Message:

    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>
  • Loading branch information
Myles Borins committed Jul 22, 2016
1 parent 3b8ea12 commit 436a055
Show file tree
Hide file tree
Showing 10 changed files with 143 additions and 73 deletions.
142 changes: 93 additions & 49 deletions lib/fs.js

Large diffs are not rendered by default.

1 change: 1 addition & 0 deletions test/fixtures/test-fs-readfile-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
require('fs').readFile('/'); // throws EISDIR
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.closeSync(fd);
fs.close(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(undefined, undefined, () => {});
fs.link();
},
/src must be a string or Buffer/
);

assert.throws(
function() {
fs.link('abc', undefined, () => {});
fs.link('abc');
},
/dest must be a string or Buffer/
);
3 changes: 3 additions & 0 deletions test/parallel/test-fs-make-callback.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ 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: 5 additions & 0 deletions test/parallel/test-fs-mkdtemp.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,3 +29,8 @@ 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: 0 additions & 17 deletions test/parallel/test-fs-no-callback-errors.js

This file was deleted.

34 changes: 34 additions & 0 deletions test/parallel/test-fs-readfile-error.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
'use strict';
var common = require('../common');
var assert = require('assert');
var exec = require('child_process').exec;
var path = require('path');

// `fs.readFile('/')` does not fail on FreeBSD, because you can open and read
// the directory there.
if (process.platform === 'freebsd') {
common.skip('platform not supported.');
return;
}

function test(env, cb) {
var filename = path.join(common.fixturesDir, 'test-fs-readfile-error.js');
var execPath = '"' + process.execPath + '" "' + filename + '"';
var options = { env: Object.assign(process.env, env) };
exec(execPath, options, function(err, stdout, stderr) {
assert(err);
assert.equal(stdout, '');
assert.notEqual(stderr, '');
cb('' + stderr);
});
}

test({ NODE_DEBUG: '' }, common.mustCall(function(data) {
assert(/EISDIR/.test(data));
assert(!/test-fs-readfile-error/.test(data));
}));

test({ NODE_DEBUG: 'fs' }, common.mustCall(function(data) {
assert(/EISDIR/.test(data));
assert(/test-fs-readfile-error/.test(data));
}));
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.closeSync(fd);
fs.close(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.closeSync(fd);
fs.close(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');
}, /"callback" argument must be a function/);
}, /"watchFile\(\)" requires a listener function/);

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

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

0 comments on commit 436a055

Please sign in to comment.