From 0631932e9472b73854e94940e3c1ebd32d49a561 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 23 Oct 2018 22:26:57 +0200 Subject: [PATCH 1/4] 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(). --- doc/api/fs.md | 10 ++++++---- lib/fs.js | 19 ++++++++++++------- test/parallel/test-fs-open.js | 32 ++++++++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 11 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index baf92fa3a20f15..7137bd7a0ffe4a 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2304,7 +2304,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` (readable and writable) * `callback` {Function} * `err` {Error} @@ -2340,7 +2341,7 @@ a colon, Node.js will open a file system stream, as described by Functions based on `fs.open()` exhibit this behavior as well: `fs.writeFile()`, `fs.readFile()`, etc. -## fs.openSync(path, flags[, mode]) +## fs.openSync(path[, flags, mode]) * `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/test/parallel/test-fs-open.js b/test/parallel/test-fs-open.js index e988a7e197bfb4..ee5a21b3d355ad 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,32 @@ 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); +})); + +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()), From 4686ff97bd74c7fe602f6c923b6487054f5cdc4e Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 23 Oct 2018 22:26:57 +0200 Subject: [PATCH 2/4] squash! doc nits --- doc/api/fs.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 7137bd7a0ffe4a..57d2d9f88ebd4b 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2318,8 +2318,8 @@ changes: --> * `path` {string|Buffer|URL} -* `flags` {string|number} **Default:** `'r'` - See [support of file system `flags`][]. +* `flags` {string|number} See [support of file system `flags`][]. + **Default:** `'r'`. * `mode` {integer} **Default:** `0o666` (readable and writable) * `callback` {Function} * `err` {Error} @@ -2352,7 +2352,7 @@ changes: --> * `path` {string|Buffer|URL} -* `flags` {string|number} **Default:** `'r'` +* `flags` {string|number} **Default:** `'r'`. See [support of file system `flags`][]. * `mode` {integer} **Default:** `0o666` * Returns: {number} From 43e6f91b42df89de1719aa800f053b134aa97480 Mon Sep 17 00:00:00 2001 From: Ben Noordhuis Date: Tue, 23 Oct 2018 22:26:58 +0200 Subject: [PATCH 3/4] squash! update fs.promises.open() --- lib/internal/fs/promises.js | 5 +++-- test/parallel/test-fs-open.js | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) 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 ee5a21b3d355ad..788124825249ca 100644 --- a/test/parallel/test-fs-open.js +++ b/test/parallel/test-fs-open.js @@ -58,6 +58,13 @@ 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()), { @@ -91,4 +98,15 @@ for (const extra of [[], ['r'], ['r', 0], ['r', 0, 'bad callback']]) { 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 + } + ); + })); }); From 079ed3386691bcd2e0bb53b3911f7a9e9b9e9062 Mon Sep 17 00:00:00 2001 From: Rich Trott Date: Thu, 25 Oct 2018 22:56:56 -0700 Subject: [PATCH 4/4] squash! lpinca nit --- doc/api/fs.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/doc/api/fs.md b/doc/api/fs.md index 57d2d9f88ebd4b..b6c58be1cf1d17 100644 --- a/doc/api/fs.md +++ b/doc/api/fs.md @@ -2304,7 +2304,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)