From 2002d90abdf91e862294daab90661aa62b4dabe3 Mon Sep 17 00:00:00 2001 From: Ian Sutherland Date: Thu, 8 Oct 2020 15:21:56 -0600 Subject: [PATCH] fs: deprecation warning on recursive rmdir This is a follow up to #35494 to add a deprecation warning when using recursive rmdir. This only warns if you are attempting to remove a file or a nonexistent path. PR-URL: https://github.com/nodejs/node/pull/35562 Reviewed-By: Matteo Collina Reviewed-By: Rich Trott Reviewed-By: Ben Coe Reviewed-By: Michael Dawson --- doc/api/deprecations.md | 7 ++-- lib/fs.js | 24 ++++++++------ lib/internal/fs/promises.js | 2 +- lib/internal/fs/utils.js | 32 +++++++++++++++++-- ...test-fs-rmdir-recursive-warns-not-found.js | 24 ++++++++++++++ .../test-fs-rmdir-recursive-warns-on-file.js | 21 ++++++++++++ 6 files changed, 95 insertions(+), 15 deletions(-) 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/doc/api/deprecations.md b/doc/api/deprecations.md index 32ddc5f16aecc3..60cf45672bebbc 100644 --- a/doc/api/deprecations.md +++ b/doc/api/deprecations.md @@ -2666,12 +2666,15 @@ changes: - version: REPLACEME pr-url: https://github.com/nodejs/node/pull/35579 description: Documentation-only deprecation. + - version: REPLACEME + pr-url: https://github.com/nodejs/node/pull/35562 + description: Runtime deprecation. --> -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. +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 diff --git a/lib/fs.js b/lib/fs.js index 8e26339e215c89..4380c4d6dd2018 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); } @@ -896,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); } @@ -906,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/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..dd2602a033f3c7 100644 --- a/lib/internal/fs/utils.js +++ b/lib/internal/fs/utils.js @@ -672,18 +672,25 @@ 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'); lazyLoadFs().stat(path, (err, stats) => { if (err) { if (options.force && err.code === 'ENOENT') { + if (warn) { + emitPermissiveRmdirWarning(); + } return callback(null, options); } return callback(err, options); } + if (warn && !stats.isDirectory()) { + emitPermissiveRmdirWarning(); + } + if (stats.isDirectory() && !options.recursive) { return callback(new ERR_FS_EISDIR({ code: 'EISDIR', @@ -697,13 +704,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 +729,29 @@ 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( + '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' + ); + permissiveRmdirWarned = true; + } +} + const validateRmdirOptions = hideStackFrames( (options, defaults = defaultRmdirOptions) => { if (options === undefined) 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..c87a20956041ce --- /dev/null +++ b/test/parallel/test-fs-rmdir-recursive-warns-not-found.js @@ -0,0 +1,24 @@ +'use strict'; +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const fs = require('fs'); +const path = require('path'); + +tmpdir.refresh(); + + +{ + // Should warn when trying to delete a nonexistent path + common.expectWarning( + 'DeprecationWarning', + '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( + 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..96b9875416f962 --- /dev/null +++ b/test/parallel/test-fs-rmdir-recursive-warns-on-file.js @@ -0,0 +1,21 @@ +'use strict'; +const common = require('../common'); +const tmpdir = require('../common/tmpdir'); +const fs = require('fs'); +const path = require('path'); + +tmpdir.refresh(); + +{ + // Should warn when trying to delete a file + common.expectWarning( + 'DeprecationWarning', + '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'); + fs.writeFileSync(filePath, ''); + fs.rmdirSync(filePath, { recursive: true }); +}