Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: deprecation warning on recursive rmdir #35562

Closed
wants to merge 11 commits into from
7 changes: 5 additions & 2 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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);
iansu marked this conversation as resolved.
Show resolved Hide resolved
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
Original file line number Diff line number Diff line change
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
Original file line number Diff line number Diff line change
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'
bcoe marked this conversation as resolved.
Show resolved Hide resolved
);
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
Original file line number Diff line number Diff line change
@@ -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
Original file line number Diff line number Diff line change
@@ -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 });
}