From 0ddd75bcd816a8e5f4e27411b8e913abb92c2ed5 Mon Sep 17 00:00:00 2001
From: Antoine du Hamel <duhamelantoine1995@gmail.com>
Date: Mon, 8 Feb 2021 12:24:06 +0100
Subject: [PATCH] fs: runtime deprecate rmdir recursive option

PR-URL: https://github.com/nodejs/node/pull/37302
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: Anto Aravinth <anto.aravinth.cse@gmail.com>
---
 doc/api/deprecations.md                       | 14 +++++---
 doc/api/fs.md                                 | 33 ++++++++++++-------
 lib/fs.js                                     |  3 ++
 lib/internal/fs/promises.js                   |  2 ++
 lib/internal/fs/utils.js                      | 24 ++++----------
 ...fs-rmdir-recursive-sync-warns-not-found.js |  3 +-
 ...t-fs-rmdir-recursive-sync-warns-on-file.js |  3 +-
 ...test-fs-rmdir-recursive-warns-not-found.js |  3 +-
 .../test-fs-rmdir-recursive-warns-on-file.js  |  3 +-
 test/parallel/test-fs-rmdir-recursive.js      |  7 ++++
 10 files changed, 53 insertions(+), 42 deletions(-)

diff --git a/doc/api/deprecations.md b/doc/api/deprecations.md
index 860d5aa749f10d..9ec966c52635f0 100644
--- a/doc/api/deprecations.md
+++ b/doc/api/deprecations.md
@@ -2671,9 +2671,12 @@ 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.
@@ -2681,9 +2684,12 @@ changes:
 
 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
diff --git a/doc/api/fs.md b/doc/api/fs.md
index d87f73a15f593f..2a6ca89873ec83 100644
--- a/doc/api/fs.md
+++ b/doc/api/fs.md
@@ -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
@@ -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`.
@@ -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
@@ -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
@@ -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`.
@@ -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
@@ -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
@@ -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`.
@@ -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
diff --git a/lib/fs.js b/lib/fs.js
index d43be885fe1e20..b02ad976bdfa4e 100644
--- a/lib/fs.js
+++ b/lib/fs.js
@@ -91,6 +91,7 @@ const internalUtil = require('internal/util');
 const {
   copyObject,
   Dirent,
+  emitRecursiveRmdirWarning,
   getDirents,
   getOptions,
   getValidatedFd,
@@ -893,6 +894,7 @@ function rmdir(path, options, callback) {
   path = pathModule.toNamespacedPath(getValidatedPath(path));
 
   if (options?.recursive) {
+    emitRecursiveRmdirWarning();
     validateRmOptions(
       path,
       { ...options, force: true },
@@ -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);
diff --git a/lib/internal/fs/promises.js b/lib/internal/fs/promises.js
index 5db2f7759c5238..26e4fd8f73be98 100644
--- a/lib/internal/fs/promises.js
+++ b/lib/internal/fs/promises.js
@@ -46,6 +46,7 @@ const { isArrayBufferView } = require('internal/util/types');
 const { rimrafPromises } = require('internal/fs/rimraf');
 const {
   copyObject,
+  emitRecursiveRmdirWarning,
   getDirents,
   getOptions,
   getStatsFromBinding,
@@ -503,6 +504,7 @@ async function rmdir(path, options) {
   options = validateRmdirOptions(options);
 
   if (options.recursive) {
+    emitRecursiveRmdirWarning();
     return rimrafPromises(path, options);
   }
 
diff --git a/lib/internal/fs/utils.js b/lib/internal/fs/utils.js
index 44f427a9f74dc5..fab2c0d68a866b 100644
--- a/lib/internal/fs/utils.js
+++ b/lib/internal/fs/utils.js
@@ -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',
@@ -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',
@@ -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;
   }
 }
 
@@ -852,6 +839,7 @@ module.exports = {
   BigIntStats,  // for testing
   copyObject,
   Dirent,
+  emitRecursiveRmdirWarning,
   getDirent,
   getDirents,
   getOptions,
diff --git a/test/parallel/test-fs-rmdir-recursive-sync-warns-not-found.js b/test/parallel/test-fs-rmdir-recursive-sync-warns-not-found.js
index 449ae0b448b353..2606a243dc87c9 100644
--- a/test/parallel/test-fs-rmdir-recursive-sync-warns-not-found.js
+++ b/test/parallel/test-fs-rmdir-recursive-sync-warns-not-found.js
@@ -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 });
diff --git a/test/parallel/test-fs-rmdir-recursive-sync-warns-on-file.js b/test/parallel/test-fs-rmdir-recursive-sync-warns-on-file.js
index 96b9875416f962..a0a5be7dbe1da7 100644
--- a/test/parallel/test-fs-rmdir-recursive-sync-warns-on-file.js
+++ b/test/parallel/test-fs-rmdir-recursive-sync-warns-on-file.js
@@ -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');
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 9f733b47d156ba..2c62f3de8141fe 100644
--- a/test/parallel/test-fs-rmdir-recursive-warns-not-found.js
+++ b/test/parallel/test-fs-rmdir-recursive-warns-not-found.js
@@ -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(
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 0d8e2e61bcbe85..c32dcdb28bd1df 100644
--- a/test/parallel/test-fs-rmdir-recursive-warns-on-file.js
+++ b/test/parallel/test-fs-rmdir-recursive-warns-on-file.js
@@ -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');
diff --git a/test/parallel/test-fs-rmdir-recursive.js b/test/parallel/test-fs-rmdir-recursive.js
index 7375eeacb07177..533a4e7c7cca22 100644
--- a/test/parallel/test-fs-rmdir-recursive.js
+++ b/test/parallel/test-fs-rmdir-recursive.js
@@ -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;