From b4b101fed63d4161df46afbe7a924c45ed63a42e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 23 Oct 2018 22:26:57 +0200 Subject: [PATCH] fs: default open/openSync flags argument to 'r' Make fs.open() and fs.openSync() more economic to use by making the flags argument optional. You can now write: fs.open(file, cb) Instead of the more verbose: fs.open(file, 'r', cb) This idiom is already supported by functions like fs.readFile(). PR-URL: https://github.com/nodejs/node/pull/23767 Reviewed-By: James M Snell Reviewed-By: Luigi Pinca Reviewed-By: Colin Ihrig Reviewed-By: Refael Ackermann Reviewed-By: Trivikram Kamat --- doc/api/fs.md | 8 +++--- lib/fs.js | 19 ++++++++----- lib/internal/fs/promises.js | 5 ++-- test/parallel/test-fs-open.js | 50 +++++++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 12 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 71ef3acc1d184e..47fcefbac07b9b 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2309,7 +2309,7 @@ this API: [`fs.mkdtemp()`][]. The optional `options` argument can be a string specifying an encoding, or an object with an `encoding` property specifying the character encoding to use. -## fs.open(path, flags[, mode], callback) +## fs.open(path[, flags[, mode]], callback) * `path` {string|Buffer|URL} -* `flags` {string|number} See [support of file system `flags`][]. +* `flags` {string|number} **Default:** `'r'`. + See [support of file system `flags`][]. * `mode` {integer} **Default:** `0o666` * Returns: {number} diff --git a/lib/fs.js b/lib/fs.js index bf964f5e9930ca..89c005375f450e 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -345,7 +345,7 @@ function tryReadSync(fd, isUserFd, buffer, pos, len) { function readFileSync(path, options) { options = getOptions(options, { flag: 'r' }); const isUserFd = isFd(path); // file descriptor ownership - const fd = isUserFd ? path : fs.openSync(path, options.flag || 'r', 0o666); + const fd = isUserFd ? path : fs.openSync(path, options.flag, 0o666); const stats = tryStatSync(fd, isUserFd); const size = isFileType(stats, S_IFREG) ? stats[8] : 0; @@ -411,14 +411,19 @@ function closeSync(fd) { function open(path, flags, mode, callback) { path = toPathIfFileURL(path); validatePath(path); - const flagsNumber = stringToFlags(flags); - if (arguments.length < 4) { - callback = makeCallback(mode); + if (arguments.length < 3) { + callback = flags; + flags = 'r'; mode = 0o666; - } else { + } else if (arguments.length === 3) { + callback = mode; + mode = 0o666; + } + const flagsNumber = stringToFlags(flags); + if (arguments.length >= 4) { mode = validateMode(mode, 'mode', 0o666); - callback = makeCallback(callback); } + callback = makeCallback(callback); const req = new FSReqCallback(); req.oncomplete = callback; @@ -433,7 +438,7 @@ function open(path, flags, mode, callback) { function openSync(path, flags, mode) { path = toPathIfFileURL(path); validatePath(path); - const flagsNumber = stringToFlags(flags); + const flagsNumber = stringToFlags(flags || 'r'); mode = validateMode(mode, 'mode', 0o666); const ctx = { path }; diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 0fecb206237100..b6b4d6605d3387 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -196,11 +196,12 @@ async function copyFile(src, dest, flags) { async function open(path, flags, mode) { path = toPathIfFileURL(path); validatePath(path); + if (arguments.length < 2) flags = 'r'; + const flagsNumber = stringToFlags(flags); mode = validateMode(mode, 'mode', 0o666); return new FileHandle( await binding.openFileHandle(pathModule.toNamespacedPath(path), - stringToFlags(flags), - mode, kUsePromises)); + flagsNumber, mode, kUsePromises)); } async function read(handle, buffer, offset, length, position) { diff --git a/test/parallel/test-fs-open.js b/test/parallel/test-fs-open.js index e988a7e197bfb4..788124825249ca 100644 --- a/test/parallel/test-fs-open.js +++ b/test/parallel/test-fs-open.js @@ -36,6 +36,12 @@ try { } assert.strictEqual(caughtException, true); +fs.openSync(__filename); + +fs.open(__filename, common.mustCall((err) => { + assert.ifError(err); +})); + fs.open(__filename, 'r', common.mustCall((err) => { assert.ifError(err); })); @@ -44,6 +50,39 @@ fs.open(__filename, 'rs', common.mustCall((err) => { assert.ifError(err); })); +fs.open(__filename, 'r', 0, common.mustCall((err) => { + assert.ifError(err); +})); + +fs.open(__filename, 'r', null, common.mustCall((err) => { + assert.ifError(err); +})); + +async function promise() { + await fs.promises.open(__filename); + await fs.promises.open(__filename, 'r'); +} + +promise().then(common.mustCall()).catch(common.mustNotCall()); + +common.expectsError( + () => fs.open(__filename, 'r', 'boom', common.mustNotCall()), + { + code: 'ERR_INVALID_ARG_VALUE', + type: TypeError + } +); + +for (const extra of [[], ['r'], ['r', 0], ['r', 0, 'bad callback']]) { + common.expectsError( + () => fs.open(__filename, ...extra), + { + code: 'ERR_INVALID_CALLBACK', + type: TypeError + } + ); +} + [false, 1, [], {}, null, undefined].forEach((i) => { common.expectsError( () => fs.open(i, 'r', common.mustNotCall()), @@ -59,4 +98,15 @@ fs.open(__filename, 'rs', common.mustCall((err) => { type: TypeError } ); + fs.promises.open(i, 'r') + .then(common.mustNotCall()) + .catch(common.mustCall((err) => { + common.expectsError( + () => { throw err; }, + { + code: 'ERR_INVALID_ARG_TYPE', + type: TypeError + } + ); + })); });