From 696d33cbc62d03326b2a0f03386130e5bdf3561f Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Thu, 8 Oct 2020 15:21:56 -0600 Subject: [PATCH 01/11] fs: deprecation warning on recursive rmdir --- lib/fs.js | 20 +++++++------ lib/internal/fs/promises.js | 2 +- lib/internal/fs/utils.js | 36 ++++++++++++++++++++++-- test/parallel/test-fs-rmdir-recursive.js | 23 +++++++++++---- 4 files changed, 64 insertions(+), 17 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 8e26339e215c89..2dc28976a63f46 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -859,14 +859,18 @@ function rmdir(path, options, callback) { path = pathModule.toNamespacedPath(getValidatedPath(path)); if (options && options.recursive) { - validateRmOptions(path, { ...options, force: true }, (err, options) => { - if (err) { - return callback(err); - } + options = validateRmOptions( + path, + { ...options, force: true }, + true, + (err, options) => { + if (err) { + return callback(err); + } - lazyLoadRimraf(); - return rimraf(path, options, callback); - }); + lazyLoadRimraf(); + return rimraf(path, options, callback); + }); } else { validateRmdirOptions(options); const req = new FSReqCallback(); @@ -879,7 +883,7 @@ function rmdirSync(path, options) { path = getValidatedPath(path); if (options && options.recursive) { - options = validateRmOptionsSync(path, { ...options, force: true }); + options = validateRmOptionsSync(path, { ...options, force: true }, true); lazyLoadRimraf(); return rimrafSync(pathModule.toNamespacedPath(path), options); } diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js index 9e1f6857d2cf96..497605aaefa72a 100644 --- a/lib/internal/fs/promises.js +++ b/lib/internal/fs/promises.js @@ -421,7 +421,7 @@ async function ftruncate(handle, len = 0) { async function rm(path, options) { path = pathModule.toNamespacedPath(getValidatedPath(path)); - options = await validateRmOptionsPromise(path, options); + options = await validateRmOptionsPromise(path, options, false); return rimrafPromises(path, options); } diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index b024ed034f8aea..ca13d4a0c4889f 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -677,13 +677,24 @@ const validateRmOptions = hideStackFrames((path, options, callback) => { validateBoolean(options.force, 'options.force'); lazyLoadFs().stat(path, (err, stats) => { - if (err) { - if (options.force && err.code === 'ENOENT') { + if (err && err.code === 'ENOENT') { + if (options.force) { + if (warn) { + emitPermissiveRmdirWarning(); + } return callback(null, options); } return callback(err, options); } + if (err) { + return callback(err); + } + + if (warn && !stats.isDirectory()) { + emitPermissiveRmdirWarning(); + } + if (stats.isDirectory() && !options.recursive) { return callback(new ERR_FS_EISDIR({ code: 'EISDIR', @@ -697,13 +708,17 @@ const validateRmOptions = hideStackFrames((path, options, callback) => { }); }); -const validateRmOptionsSync = hideStackFrames((path, options) => { +const validateRmOptionsSync = hideStackFrames((path, options, warn) => { options = validateRmdirOptions(options, defaultRmOptions); validateBoolean(options.force, 'options.force'); try { const stats = lazyLoadFs().statSync(path); + if (warn && !stats.isDirectory()) { + emitPermissiveRmdirWarning(); + } + if (stats.isDirectory() && !options.recursive) { throw new ERR_FS_EISDIR({ code: 'EISDIR', @@ -718,12 +733,27 @@ const validateRmOptionsSync = hideStackFrames((path, options) => { throw err; } else if (err.code === 'ENOENT' && !options.force) { throw err; + } else if (warn && err.code === 'ENOENT') { + emitPermissiveRmdirWarning(); } } return options; }); +let permissiveRmdirWarned = false; + +function emitPermissiveRmdirWarning() { + if (!permissiveRmdirWarned) { + process.emitWarning( + 'Permissive rmdir recursive is deprecated, use rm recursive instead', + 'DeprecationWarning', + 'DEP0147' + ); + permissiveRmdirWarned = true; + } +} + const validateRmdirOptions = hideStackFrames( (options, defaults = defaultRmdirOptions) => { if (options === undefined) diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index 1d2525a5ed6341..ce6ee4e8de5f3e 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -7,6 +7,14 @@ const fs = require('fs'); const path = require('path'); const { validateRmdirOptions } = require('internal/fs/utils'); +const realEmitWarning = process.emitWarning; +process.emitWarning = () => { + // Reset back to default after the test. + process.emitWarning = realEmitWarning; + + throw new Error('deprecated'); +}; + tmpdir.refresh(); let count = 0; @@ -75,7 +83,8 @@ function removeAsync(dir) { fs.rmdir(dir, { recursive: true }, common.mustCall((err) => { assert.ifError(err); - // No error should occur if recursive and the directory does not exist. + // No deprecation warning should occur if recursive and the directory + // does not exist because the warning has already been output once. fs.rmdir(dir, { recursive: true }, common.mustCall((err) => { assert.ifError(err); @@ -123,8 +132,11 @@ function removeAsync(dir) { // Recursive removal should succeed. fs.rmdirSync(dir, { recursive: true }); - // No error should occur if recursive and the directory does not exist. - fs.rmdirSync(dir, { recursive: true }); + // Should print deprecation warning if recursive and the directory does not + // exist. + assert.throws(() => { + fs.rmdirSync(dir, { recursive: true }); + }, { message: 'deprecated' }); // Attempted removal should fail now because the directory is gone. assert.throws(() => fs.rmdirSync(dir), { syscall: 'rmdir' }); @@ -144,8 +156,9 @@ function removeAsync(dir) { // Recursive removal should succeed. await fs.promises.rmdir(dir, { recursive: true }); - // No error should occur if recursive and the directory does not exist. - await fs.promises.rmdir(dir, { recursive: true }); + // No deprecation warning should occur if recursive and the directory does not + // exist because the warning has already been output once. + fs.promises.rmdir(dir, { recursive: true }); // Attempted removal should fail now because the directory is gone. assert.rejects(fs.promises.rmdir(dir), { syscall: 'rmdir' }); From 1457c495772f145cf120665f653babbbaee507fc Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Thu, 8 Oct 2020 15:36:15 -0600 Subject: [PATCH 02/11] Use expect warning test helper --- test/parallel/test-fs-rmdir-recursive.js | 28 ++++++++++-------------- 1 file changed, 11 insertions(+), 17 deletions(-) diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index ce6ee4e8de5f3e..520005f395f12e 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -7,16 +7,14 @@ const fs = require('fs'); const path = require('path'); const { validateRmdirOptions } = require('internal/fs/utils'); -const realEmitWarning = process.emitWarning; -process.emitWarning = () => { - // Reset back to default after the test. - process.emitWarning = realEmitWarning; - - throw new Error('deprecated'); -}; - tmpdir.refresh(); +common.expectWarning( + 'DeprecationWarning', + 'Permissive rmdir recursive is deprecated, use rm recursive instead', + 'DEP0147' +); + let count = 0; const nextDirPath = (name = 'rmdir-recursive') => path.join(tmpdir.path, `${name}-${count++}`); @@ -83,8 +81,8 @@ function removeAsync(dir) { fs.rmdir(dir, { recursive: true }, common.mustCall((err) => { assert.ifError(err); - // No deprecation warning should occur if recursive and the directory - // does not exist because the warning has already been output once. + // Should print deprecation warning if recursive and directory does not + // exist. fs.rmdir(dir, { recursive: true }, common.mustCall((err) => { assert.ifError(err); @@ -132,11 +130,8 @@ function removeAsync(dir) { // Recursive removal should succeed. fs.rmdirSync(dir, { recursive: true }); - // Should print deprecation warning if recursive and the directory does not - // exist. - assert.throws(() => { - fs.rmdirSync(dir, { recursive: true }); - }, { message: 'deprecated' }); + // Should print deprecation warning if recursive and directory does not exist. + fs.rmdirSync(dir, { recursive: true }); // Attempted removal should fail now because the directory is gone. assert.throws(() => fs.rmdirSync(dir), { syscall: 'rmdir' }); @@ -156,8 +151,7 @@ function removeAsync(dir) { // Recursive removal should succeed. await fs.promises.rmdir(dir, { recursive: true }); - // No deprecation warning should occur if recursive and the directory does not - // exist because the warning has already been output once. + // Should print deprecation warning if recursive and directory does not exist. fs.promises.rmdir(dir, { recursive: true }); // Attempted removal should fail now because the directory is gone. From ee4d3dae06ab333171e8c286f624d889741c4e91 Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Thu, 8 Oct 2020 19:05:17 -0600 Subject: [PATCH 03/11] Code review changes --- lib/fs.js | 4 ++-- lib/internal/fs/utils.js | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/fs.js b/lib/fs.js index 2dc28976a63f46..4380c4d6dd2018 100644 --- a/lib/fs.js +++ b/lib/fs.js @@ -900,7 +900,7 @@ function rm(path, options, callback) { options = undefined; } - validateRmOptions(path, options, (err, options) => { + validateRmOptions(path, options, false, (err, options) => { if (err) { return callback(err); } @@ -910,7 +910,7 @@ function rm(path, options, callback) { } function rmSync(path, options) { - options = validateRmOptionsSync(path, options); + options = validateRmOptionsSync(path, options, false); lazyLoadRimraf(); return rimrafSync(pathModule.toNamespacedPath(path), options); diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index ca13d4a0c4889f..d857fdbfea2a12 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -746,7 +746,8 @@ let permissiveRmdirWarned = false; function emitPermissiveRmdirWarning() { if (!permissiveRmdirWarned) { process.emitWarning( - 'Permissive rmdir recursive is deprecated, use rm recursive instead', + 'Permissive rmdir recursive is deprecated, use rm recursive and force\ +instead', 'DeprecationWarning', 'DEP0147' ); From f3d67e60a3655ab22e16f957978ccf9b8bb1f50b Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Fri, 9 Oct 2020 13:46:34 -0600 Subject: [PATCH 04/11] Update deprecation message in tests --- lib/internal/fs/utils.js | 2 +- test/parallel/test-fs-rmdir-recursive.js | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index d857fdbfea2a12..1f199f151f5552 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -746,7 +746,7 @@ let permissiveRmdirWarned = false; function emitPermissiveRmdirWarning() { if (!permissiveRmdirWarned) { process.emitWarning( - 'Permissive rmdir recursive is deprecated, use rm recursive and force\ + 'Permissive rmdir recursive is deprecated, use rm recursive and force \ instead', 'DeprecationWarning', 'DEP0147' diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js index 520005f395f12e..39c15cb4a847d1 100644 --- a/test/parallel/test-fs-rmdir-recursive.js +++ b/test/parallel/test-fs-rmdir-recursive.js @@ -11,7 +11,8 @@ tmpdir.refresh(); common.expectWarning( 'DeprecationWarning', - 'Permissive rmdir recursive is deprecated, use rm recursive instead', + 'Permissive rmdir recursive is deprecated, use rm recursive and force \ +instead', 'DEP0147' ); From 968e299b2cbfa87c0a0b8da5f132c411937903de Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Mon, 12 Oct 2020 15:12:41 -0600 Subject: [PATCH 05/11] Improve tests --- ...test-fs-rmdir-recursive-warns-not-found.js | 24 +++++++++++++++++++ .../test-fs-rmdir-recursive-warns-on-file.js | 23 ++++++++++++++++++ 2 files changed, 47 insertions(+) create mode 100644 test/parallel/test-fs-rmdir-recursive-warns-not-found.js create mode 100644 test/parallel/test-fs-rmdir-recursive-warns-on-file.js diff --git a/test/parallel/test-fs-rmdir-recursive-warns-not-found.js b/test/parallel/test-fs-rmdir-recursive-warns-not-found.js new file mode 100644 index 00000000000000..81f69b8a90c137 --- /dev/null +++ b/test/parallel/test-fs-rmdir-recursive-warns-not-found.js @@ -0,0 +1,24 @@ +// Flags: --expose-internals +'use strict'; +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const fs = require('fs'); +const path = require('path'); + +tmpdir.refresh(); + +common.expectWarning( + 'DeprecationWarning', + 'Permissive rmdir recursive is deprecated, use rm recursive and force \ +instead', + 'DEP0147' +); + +{ + // Should warn when trying to delete a nonexistent path + fs.rmdir( + path.join(tmpdir.path, 'noexist.txt'), + { recursive: true }, + common.mustCall() + ); +} diff --git a/test/parallel/test-fs-rmdir-recursive-warns-on-file.js b/test/parallel/test-fs-rmdir-recursive-warns-on-file.js new file mode 100644 index 00000000000000..d43e28caa5f448 --- /dev/null +++ b/test/parallel/test-fs-rmdir-recursive-warns-on-file.js @@ -0,0 +1,23 @@ +// Flags: --expose-internals +'use strict'; +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const fs = require('fs'); +const path = require('path'); + +tmpdir.refresh(); + +common.expectWarning( + 'DeprecationWarning', + 'Permissive rmdir recursive is deprecated, use rm recursive and force \ +instead', + 'DEP0147' +); + +{ + const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt'); + fs.writeFileSync(filePath, ''); + + // Should warn when trying to delete a file + fs.rmdirSync(filePath, { recursive: true }); +} From 327e72487841f49d737c898eae68b271f4ee67df Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Mon, 12 Oct 2020 15:15:34 -0600 Subject: [PATCH 06/11] Update deprecation --- doc/api/deprecations.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 32ddc5f16aecc3..94188e6b0bb627 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2664,15 +2664,15 @@ The [`crypto.Certificate()` constructor][] is deprecated. Use Type: Documentation-only In future versions of Node.js, `fs.rmdir(path, { recursive: true })` will throw -on nonexistent paths, or when given a file as a target. -Use `fs.rm(path, { recursive: true, force: true })` instead. +on nonexistent paths, or when given a file as a target, use +`fs.rm(path, {recursive: true, force: true })` instead. [Legacy URL API]: url.md#url_legacy_url_api [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf From 197b4390c17ffe474472c91637b667405b64f5a5 Mon Sep 17 00:00:00 2001 From: bcoe Date: Tue, 13 Oct 2020 07:47:35 -0700 Subject: [PATCH 07/11] address review nits --- doc/api/deprecations.md | 3 +++ lib/internal/fs/utils.js | 8 ++------ .../test-fs-rmdir-recursive-warns-not-found.js | 16 +++++++--------- .../test-fs-rmdir-recursive-warns-on-file.js | 17 +++++++---------- test/parallel/test-fs-rmdir-recursive.js | 14 +++----------- 5 files changed, 22 insertions(+), 36 deletions(-) diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md index 94188e6b0bb627..63555d8d8776e1 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2663,6 +2663,9 @@ The [`crypto.Certificate()` constructor][] is deprecated. Use ### DEP0147: `fs.rmdir(path, { recursive: true })` -Type: Documentation-only +Type: Runtime In future versions of Node.js, `fs.rmdir(path, { recursive: true })` will throw -on nonexistent paths, or when given a file as a target, use -`fs.rm(path, {recursive: true, force: true })` instead. +if `path` does not exist or is a file. +Use `fs.rm(path, { recursive: true, force: true })` instead. [Legacy URL API]: url.md#url_legacy_url_api [NIST SP 800-38D]: https://nvlpubs.nist.gov/nistpubs/Legacy/SP/nistspecialpublication800-38d.pdf From 2fd90bd1cb83e3702bdfe1d3dbaabdcc1d906022 Mon Sep 17 00:00:00 2001 From: bcoe Date: Tue, 13 Oct 2020 08:45:09 -0700 Subject: [PATCH 10/11] chore: address merge issues --- lib/internal/fs/utils.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index d96961519a568c..e5c1b92a3e5c84 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -672,7 +672,7 @@ const defaultRmdirOptions = { recursive: false, }; -const validateRmOptions = hideStackFrames((path, options, callback) => { +const validateRmOptions = hideStackFrames((path, options, warn, callback) => { options = validateRmdirOptions(options, defaultRmOptions); validateBoolean(options.force, 'options.force'); From 5978379f165681e2a5f5bb0f857894bdea1aaad8 Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Wed, 14 Oct 2020 10:29:37 -0600 Subject: [PATCH 11/11] Update deprecation message --- lib/internal/fs/utils.js | 5 +++-- test/parallel/test-fs-rmdir-recursive-warns-not-found.js | 8 +++++--- test/parallel/test-fs-rmdir-recursive-warns-on-file.js | 7 ++++--- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js index e5c1b92a3e5c84..dd2602a033f3c7 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -742,8 +742,9 @@ let permissiveRmdirWarned = false; function emitPermissiveRmdirWarning() { if (!permissiveRmdirWarned) { process.emitWarning( - 'Permissive rmdir recursive is deprecated, use rm recursive and force \ -instead', + 'In future versions of Node.js, fs.rmdir(path, { recursive: true }) ' + + 'will throw if path does not exist or is a file. Use fs.rm(path, ' + + '{ recursive: true, force: true }) instead', 'DeprecationWarning', 'DEP0147' ); diff --git a/test/parallel/test-fs-rmdir-recursive-warns-not-found.js b/test/parallel/test-fs-rmdir-recursive-warns-not-found.js index f7042501ce2913..c87a20956041ce 100644 --- a/test/parallel/test-fs-rmdir-recursive-warns-not-found.js +++ b/test/parallel/test-fs-rmdir-recursive-warns-not-found.js @@ -6,12 +6,14 @@ const path = require('path'); tmpdir.refresh(); -// Should warn when trying to delete a nonexistent path + { + // Should warn when trying to delete a nonexistent path common.expectWarning( 'DeprecationWarning', - 'Permissive rmdir recursive is deprecated, use rm recursive and force \ -instead', + 'In future versions of Node.js, fs.rmdir(path, { recursive: true }) ' + + 'will throw if path does not exist or is a file. Use fs.rm(path, ' + + '{ recursive: true, force: true }) instead', 'DEP0147' ); fs.rmdir( diff --git a/test/parallel/test-fs-rmdir-recursive-warns-on-file.js b/test/parallel/test-fs-rmdir-recursive-warns-on-file.js index b52528498fa845..96b9875416f962 100644 --- a/test/parallel/test-fs-rmdir-recursive-warns-on-file.js +++ b/test/parallel/test-fs-rmdir-recursive-warns-on-file.js @@ -6,12 +6,13 @@ const path = require('path'); tmpdir.refresh(); -// Should warn when trying to delete a file { + // Should warn when trying to delete a file common.expectWarning( 'DeprecationWarning', - 'Permissive rmdir recursive is deprecated, use rm recursive and force \ -instead', + 'In future versions of Node.js, fs.rmdir(path, { recursive: true }) ' + + 'will throw if path does not exist or is a file. Use fs.rm(path, ' + + '{ recursive: true, force: true }) instead', 'DEP0147' ); const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');