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: runtime deprecate rmdir recursive option #37302

Merged
merged 1 commit into from
Mar 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 10 additions & 4 deletions doc/api/deprecations.md
Original file line number Diff line number Diff line change
Expand Up @@ -2671,19 +2671,25 @@ The [`crypto.Certificate()` constructor][] is deprecated. Use
### DEP0147: `fs.rmdir(path, { recursive: true })`
<!-- YAML
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37302
description: Runtime deprecation.
- version: v15.0.0
pr-url: https://github.com/nodejs/node/pull/35562
description: Runtime deprecation.
description: Runtime deprecation for permissive behavior.
- version: v14.14.0
pr-url: https://github.com/nodejs/node/pull/35579
description: Documentation-only deprecation.
-->

Type: Runtime

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.
In future versions of Node.js, `recursive` option will be ignored for
`fs.rmdir`, `fs.rmdirSync`, and `fs.promises.rmdir`.

Use `fs.rm(path, { recursive: true, force: true })`,
`fs.rmSync(path, { recursive: true, force: true })` or
`fs.promises.rm(path, { recursive: true, force: true })` instead.

### DEP0148: Folder mappings in `"exports"` (trailing `"/"`)
<!-- YAML
Expand Down
33 changes: 21 additions & 12 deletions doc/api/fs.md
Original file line number Diff line number Diff line change
Expand Up @@ -1048,6 +1048,10 @@ Renames `oldPath` to `newPath`.
<!-- YAML
added: v10.0.0
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37302
description: The `recursive` option is deprecated, using it triggers a
deprecation warning.
- version:
- v13.3.0
- v12.16.0
Expand All @@ -1072,7 +1076,7 @@ changes:
option is not `true`. **Default:** `0`.
* `recursive` {boolean} If `true`, perform a recursive directory removal. In
recursive mode, errors are not reported if `path` does not exist, and
operations are retried on failure. **Default:** `false`.
operations are retried on failure. **Default:** `false`. **Deprecated**.
* `retryDelay` {integer} The amount of time in milliseconds to wait between
retries. This option is ignored if the `recursive` option is not `true`.
**Default:** `100`.
Expand All @@ -1086,9 +1090,8 @@ error on POSIX.

Setting `recursive` to `true` results in behavior similar to the Unix command
`rm -rf`: an error will not be raised for paths that do not exist, and paths
that represent files will be deleted. The permissive behavior of the
`recursive` option is deprecated, `ENOTDIR` and `ENOENT` will be thrown in
the future.
that represent files will be deleted. The `recursive` option is deprecated,
`ENOTDIR` and `ENOENT` will be thrown in the future.

### `fsPromises.rm(path[, options])`
<!-- YAML
Expand Down Expand Up @@ -3135,6 +3138,10 @@ rename('oldFile.txt', 'newFile.txt', (err) => {
<!-- YAML
added: v0.0.2
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37302
description: The `recursive` option is deprecated, using it triggers a
deprecation warning.
- version:
- v13.3.0
- v12.16.0
Expand Down Expand Up @@ -3171,7 +3178,7 @@ changes:
option is not `true`. **Default:** `0`.
* `recursive` {boolean} If `true`, perform a recursive directory removal. In
recursive mode, errors are not reported if `path` does not exist, and
operations are retried on failure. **Default:** `false`.
operations are retried on failure. **Default:** `false`. **Deprecated**.
* `retryDelay` {integer} The amount of time in milliseconds to wait between
retries. This option is ignored if the `recursive` option is not `true`.
**Default:** `100`.
Expand All @@ -3186,9 +3193,8 @@ Windows and an `ENOTDIR` error on POSIX.

Setting `recursive` to `true` results in behavior similar to the Unix command
`rm -rf`: an error will not be raised for paths that do not exist, and paths
that represent files will be deleted. The permissive behavior of the
`recursive` option is deprecated, `ENOTDIR` and `ENOENT` will be thrown in
the future.
that represent files will be deleted. The `recursive` option is deprecated,
`ENOTDIR` and `ENOENT` will be thrown in the future.

### `fs.rm(path[, options], callback)`
<!-- YAML
Expand Down Expand Up @@ -4755,6 +4761,10 @@ See the POSIX rename(2) documentation for more details.
<!-- YAML
added: v0.1.21
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/37302
description: The `recursive` option is deprecated, using it triggers a
deprecation warning.
- version:
- v13.3.0
- v12.16.0
Expand Down Expand Up @@ -4783,7 +4793,7 @@ changes:
option is not `true`. **Default:** `0`.
* `recursive` {boolean} If `true`, perform a recursive directory removal. In
recursive mode, errors are not reported if `path` does not exist, and
operations are retried on failure. **Default:** `false`.
operations are retried on failure. **Default:** `false`. **Deprecated**.
* `retryDelay` {integer} The amount of time in milliseconds to wait between
retries. This option is ignored if the `recursive` option is not `true`.
**Default:** `100`.
Expand All @@ -4795,9 +4805,8 @@ on Windows and an `ENOTDIR` error on POSIX.

Setting `recursive` to `true` results in behavior similar to the Unix command
`rm -rf`: an error will not be raised for paths that do not exist, and paths
that represent files will be deleted. The permissive behavior of the
`recursive` option is deprecated, `ENOTDIR` and `ENOENT` will be thrown in
the future.
that represent files will be deleted. The `recursive` option is deprecated,
`ENOTDIR` and `ENOENT` will be thrown in the future.

### `fs.rmSync(path[, options])`
<!-- YAML
Expand Down
3 changes: 3 additions & 0 deletions lib/fs.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ const internalUtil = require('internal/util');
const {
copyObject,
Dirent,
emitRecursiveRmdirWarning,
getDirents,
getOptions,
getValidatedFd,
Expand Down Expand Up @@ -893,6 +894,7 @@ function rmdir(path, options, callback) {
path = pathModule.toNamespacedPath(getValidatedPath(path));

if (options?.recursive) {
emitRecursiveRmdirWarning();
validateRmOptions(
path,
{ ...options, force: true },
Expand All @@ -917,6 +919,7 @@ function rmdirSync(path, options) {
path = getValidatedPath(path);

if (options?.recursive) {
emitRecursiveRmdirWarning();
options = validateRmOptionsSync(path, { ...options, force: true }, true);
lazyLoadRimraf();
return rimrafSync(pathModule.toNamespacedPath(path), options);
Expand Down
2 changes: 2 additions & 0 deletions lib/internal/fs/promises.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ const { isArrayBufferView } = require('internal/util/types');
const { rimrafPromises } = require('internal/fs/rimraf');
const {
copyObject,
emitRecursiveRmdirWarning,
getDirents,
getOptions,
getStatsFromBinding,
Expand Down Expand Up @@ -503,6 +504,7 @@ async function rmdir(path, options) {
options = validateRmdirOptions(options);

if (options.recursive) {
emitRecursiveRmdirWarning();
return rimrafPromises(path, options);
}

Expand Down
24 changes: 6 additions & 18 deletions lib/internal/fs/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -705,18 +705,11 @@ const validateRmOptions = hideStackFrames((path, options, warn, callback) => {
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 @@ -738,10 +731,6 @@ const validateRmOptionsSync = hideStackFrames((path, options, warn) => {
const isDirectory = lazyLoadFs()
.statSync(path, { throwIfNoEntry: !options.force })?.isDirectory();

if (warn && !isDirectory) {
emitPermissiveRmdirWarning();
}

if (isDirectory && !options.recursive) {
throw new ERR_FS_EISDIR({
code: 'EISDIR',
Expand All @@ -756,18 +745,16 @@ const validateRmOptionsSync = hideStackFrames((path, options, warn) => {
return options;
});

let permissiveRmdirWarned = false;

function emitPermissiveRmdirWarning() {
if (!permissiveRmdirWarned) {
let recursiveRmdirWarned = process.noDeprecation;
function emitRecursiveRmdirWarning() {
if (!recursiveRmdirWarned) {
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',
'will be removed. Use fs.rm(path, { recursive: true }) instead',
'DeprecationWarning',
'DEP0147'
);
permissiveRmdirWarned = true;
recursiveRmdirWarned = true;
}
}

Expand Down Expand Up @@ -852,6 +839,7 @@ module.exports = {
BigIntStats, // for testing
copyObject,
Dirent,
emitRecursiveRmdirWarning,
getDirent,
getDirents,
getOptions,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ tmpdir.refresh();
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',
'will be removed. Use fs.rm(path, { recursive: true }) instead',
'DEP0147'
);
fs.rmdirSync(path.join(tmpdir.path, 'noexist.txt'), { recursive: true });
Expand Down
3 changes: 1 addition & 2 deletions test/parallel/test-fs-rmdir-recursive-sync-warns-on-file.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ tmpdir.refresh();
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',
'will be removed. Use fs.rm(path, { recursive: true }) instead',
'DEP0147'
);
const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');
Expand Down
3 changes: 1 addition & 2 deletions test/parallel/test-fs-rmdir-recursive-warns-not-found.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ tmpdir.refresh();
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',
'will be removed. Use fs.rm(path, { recursive: true }) instead',
'DEP0147'
);
fs.rmdir(
Expand Down
3 changes: 1 addition & 2 deletions test/parallel/test-fs-rmdir-recursive-warns-on-file.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ tmpdir.refresh();
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',
'will be removed. Use fs.rm(path, { recursive: true }) instead',
'DEP0147'
);
const filePath = path.join(tmpdir.path, 'rmdir-recursive.txt');
Expand Down
7 changes: 7 additions & 0 deletions test/parallel/test-fs-rmdir-recursive.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ const fs = require('fs');
const path = require('path');
const { validateRmdirOptions } = require('internal/fs/utils');

common.expectWarning(
'DeprecationWarning',
'In future versions of Node.js, fs.rmdir(path, { recursive: true }) ' +
'will be removed. Use fs.rm(path, { recursive: true }) instead',
'DEP0147'
);

tmpdir.refresh();

let count = 0;
Expand Down