Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
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: #35562
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Michael Dawson <midawson@redhat.com>
  • Loading branch information
iansu authored and bcoe committed Oct 15, 2020
1 parent ab0af50 commit 2002d90
Show file tree
Hide file tree
Showing 6 changed files with 95 additions and 15 deletions.
7 changes: 5 additions & 2 deletions doc/api/deprecations.md
Expand Up @@ -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
Expand Down
24 changes: 14 additions & 10 deletions lib/fs.js
Expand Up @@ -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();
Expand All @@ -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);
}
Expand All @@ -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);
}
Expand All @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/fs/promises.js
Expand Up @@ -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);
}

Expand Down
32 changes: 30 additions & 2 deletions lib/internal/fs/utils.js
Expand Up @@ -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',
Expand All @@ -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',
Expand All @@ -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)
Expand Down
24 changes: 24 additions & 0 deletions 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()
);
}
21 changes: 21 additions & 0 deletions 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 });
}

0 comments on commit 2002d90

Please sign in to comment.