fs: deprecation warning on recursive rmdir #35562

iansu · 2020-10-08T21:23:48Z

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.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

doc/api/deprecations.md

BridgeAR · 2020-10-08T21:29:40Z

test/parallel/test-fs-rmdir-recursive.js

+
+  throw new Error('deprecated');
+};
+


We do have a helper to check for deprecation messages. Please check expectWarning from the common test utils. There's also a documentation for it in the common README.

That's great. I was hoping there was a better way to do this.

bcoe

This is looking good to me.

One nit about the test, I'm wondering if we could make it more clear that we're testing multiple test cases where a warning should be emitted.

lib/internal/fs/utils.js

test/parallel/test-fs-rmdir-recursive.js

lib/fs.js

mcollina

lgtm

aduh95 · 2020-10-09T13:14:34Z

Would it be worth to have a PR with a doc-only deprecation that can be backported to v14.x first?

bcoe · 2020-10-09T16:01:10Z

Would it be worth to have a PR with a doc-only deprecation that can be backported to v14.x first?

I'll add a docs only deprecation to deprecations.md in a separate PR, I missed doing this in #35171.

doc/api/deprecations.md

Trott

Looks like test failures are relevant?

bcoe · 2020-10-09T22:08:06Z

@Trott something jumped out at me while writing the docs only deprecation message:

Should this be a Run-time deprecation or an end-of-life deprecation? wasn't quite clear on how we'd decide on which. I expect in v16 we will remove the permissive behavior, based on the current plan.

Trott · 2020-10-09T22:58:48Z

@Trott something jumped out at me while writing the docs only deprecation message:

Should this be a Run-time deprecation or an end-of-life deprecation? wasn't quite clear on how we'd decide on which. I expect in v16 we will remove the permissive behavior, based on the current plan.

So in 15.x it will emit a warning and in 16.x it will adopt the less permissive behavior? If so, seems like runtime for 15.x and then EOL for 16.x.

bcoe · 2020-10-10T01:05:38Z

So in 15.x it will emit a warning and in 16.x it will adopt the less permissive behavior?

Correct, sounds like a plan seems like runtime for 15.x and then EOL for 16.x..

doc/api/deprecations.md

Trott · 2020-10-10T13:06:28Z

@nodejs/tsc IIUC, this is a semver-major that we hope to get into the 15.0.0 release. It has two TSC approvals, but it would be good to get a few more reviews.

nodejs-github-bot · 2020-10-10T13:06:35Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33514/

Trott · 2020-10-10T13:07:15Z

Once Jenkins CI runs, I imagine a CITGM run is in order.

nodejs-github-bot · 2020-10-10T16:54:28Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33520/

Document deprecation of rmdir/recursive permissive functionality PR-URL: #35579 Refs: #35562 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

doc/api/deprecations.md

nodejs-github-bot · 2020-10-13T17:48:31Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33593/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2513/

bcoe · 2020-10-13T17:57:07Z

@mhdawson, @BridgeAR, @nodejs/tsc gentle nudge, to make sure this is in the spirt of the deprecation we discussed on the TSC call the other week, and that all comments are addressed.

I'd like to make sure we have a healthy number of 👀 on this, since this is a breaking change.

mhdawson

LGTM

bcoe · 2020-10-13T22:15:09Z

@BethGriggs @Trott any advice on how to best digest a CITGM run, seems like we have a healthy resting number of errors.

Is reading through the output, looking for anything that jumps out related to rimraf sufficient?

lib/internal/fs/utils.js

Trott · 2020-10-14T15:02:17Z

Is reading through the output, looking for anything that jumps out related to rimraf sufficient?

Different people probably handle it differently, but I run a CITGM against master as a baseline, and only investigate items in the PR run that aren't in the master run.

Trott · 2020-10-14T15:03:51Z

CITGM on master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2516/

CITGM on this PR rebased onto master: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/2517/ (queued, will 404 until it starts running, which probably won't be until the job above finishes)

nodejs-github-bot · 2020-10-14T18:11:25Z

CI: https://ci.nodejs.org/job/node-test-pull-request/33622/

Document deprecation of rmdir/recursive permissive functionality PR-URL: #35579 Refs: #35562 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

bcoe · 2020-10-14T23:06:38Z

@Trott @iansu digging in to the delta in the two canary runs, I saw four additional failures:

citgm.ava-v3.13.0
citgm.multer-v1.4.2
citgm.socket.io-v2.3.0
citgm.q-v1.5.1

When I dug into each of these, it wasn't obvious to me that they were related to the deprecation, they were all timeouts, or other issues that looked somewhat intermittent. thoughts?

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 <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

bcoe · 2020-10-15T01:59:09Z

Landed in 2002d90

Notable changes: Deprecations and Removals: - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **timers**: introduce timers/promises (James M Snell) (#33950) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) npm 7 (#35631): Node.js 15 comes with a new major release of npm, npm 7. npm 7 comes with many new features - including npm workspaces and a new package-lock.json format. npm 7 also includes yarn.lock file support. One of the big changes in npm 7 is that peer dependencies are now installed by default. Throw On Unhandled Rejections (#33021): As of Node.js 15, the default mode for `unhandledRejection` is changed to `throw` (from `warn`). In `throw` mode, if an `unhandledRejection` hook is not set, the `unhandledRejection` is raised as an uncaught exception. Users that have an `unhandledRejection` hook should see no change in behavior, and it’s still possible to switch modes using the `--unhandled-rejections=mode` process flag. QUIC (#32379): Node.js 15 comes with experimental support QUIC, which can be enabled by compiling Node.js with the `--experimental-quic` configuration flag. The Node.js QUIC implementation is exposed by the core `net` module. V8 8.6 (#35415): The V8 JavaScript engine has been updated to V8 8.6 (V8 8.4 is the latest available in Node.js 14). Along with performance tweaks and improvements the V8 update also brings the following language features: * `Promise.any()` (from V8 8.5) * `AggregateError` (from V8 8.5) * `String.prototype.replaceAll()` (from V8 8.5) * Logical assignment operators `&&=`, `||=`, and `??=` (from V8 8.5) Other Notable Changes: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **dns**: add dns/promises alias (shisama) (#32953) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **stream**: construct (Robert Nagy) (#29656) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) Semver-Major Commits: - **assert**: add `assert/strict` alias module (ExE Boss) (#34001) - **build**: reset embedder string to "-node.0" (Michaël Zasso) (#35415) - **build**: remove --build-v8-with-gn configure option (Yang Guo) (#27576) - **build**: drop support for VS2017 (Michaël Zasso) (#33694) - **crypto**: refactoring internals, add WebCrypto (James M Snell) (#35093) - **crypto**: move node\_crypto files to src/crypto (James M Snell) (#35093) - **deps**: V8: cherry-pick d76abfed3512 (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 717543bbf0ef (Michaël Zasso) (#35415) - **deps**: V8: cherry-pick 6be2f6e26e8d (Michaël Zasso) (#35415) - **deps**: fix V8 build issue with inline methods (Jiawen Geng) (#35415) - **deps**: fix platform-embedded-file-writer-win for ARM64 (Michaël Zasso) (#35415) - **deps**: update V8 postmortem metadata script (Colin Ihrig) (#35415) - **deps**: update V8 to 8.6.395 (Michaël Zasso) (#35415) - **deps**: upgrade npm to 7.0.0 (Myles Borins) (#35631) - **deps**: update npm to 7.0.0-rc.3 (Myles Borins) (#35474) - **deps**: V8: cherry-pick 0d6debcc5f08 (Gus Caplan) (#33600) - **dns**: add dns/promises alias (shisama) (#32953) - **doc**: move DEP0018 to End-of-Life (Rich Trott) (#35316) - **doc**: update support macos version for 15.x (Ash Cripps) (#35022) - **fs**: deprecation warning on recursive rmdir (Ian Sutherland) (#35562) - **fs**: reimplement read and write streams using stream.construct (Robert Nagy) (#29656) - **http**: fixed socket.setEncoding fatal error (iskore) (#33405) - **http**: emit 'error' on aborted server request (Robert Nagy) (#33172) - **http**: cleanup end argument handling (Robert Nagy) (#31818) - **http2**: allow Host in HTTP/2 requests (Alba Mendez) (#34664) - **http2**: add `invalidheaders` test (Pranshu Srivastava) (#33161) - **http2**: refactor state code validation for the http2Stream class (rickyes) (#33535) - **http2**: header field valid checks (Pranshu Srivastava) (#33193) - **lib**: add EventTarget-related browser globals (Anna Henningsen) (#35496) - **lib**: remove ERR\_INVALID\_OPT\_VALUE and ERR\_INVALID\_OPT\_VALUE\_ENCODING (Denys Otrishko) (#34682) - **lib**: handle one of args case in ERR\_MISSING\_ARGS (Denys Otrishko) (#34022) - **lib**: remove NodeError from the prototype of errors with code (Michaël Zasso) (#33857) - **lib**: unflag AbortController (James M Snell) (#33527) - **lib**: initial experimental AbortController implementation (James M Snell) (#33527) - **net**: check args in net.connect() and socket.connect() calls (Denys Otrishko) (#34022) - **net**: remove long deprecated server.connections property (James M Snell) (#33647) - **net**: autoDestroy Socket (Robert Nagy) (#31806) - **process**: update v8 fast api calls usage (Maya Lekova) (#35415) - **process**: change default --unhandled-rejections=throw (Dan Fabulich) (#33021) - **process**: use v8 fast api calls for hrtime (Gus Caplan) (#33600) - **process**: delay throwing an error using `throwDeprecation` (Ruben Bridgewater) (#32312) - **repl**: remove deprecated repl.memory function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.turnOffEditorMode() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated repl.parseREPLKeyword() function (Ruben Bridgewater) (#33286) - **repl**: remove deprecated bufferedCommand property (Ruben Bridgewater) (#33286) - **repl**: remove deprecated .rli (Ruben Bridgewater) (#33286) - **src**: implement NodePlatform::PostJob (Clemens Backes) (#35415) - **src**: update NODE\_MODULE\_VERSION to 88 (Michaël Zasso) (#35415) - **src**: error reporting on CPUUsage (Yash Ladha) (#34762) - **src**: use node:moduleName as builtin module filename (Michaël Zasso) (#35498) - **src**: enable wasm trap handler on windows (Gus Caplan) (#35033) - **src**: update NODE\_MODULE\_VERSION to 86 (Michaël Zasso) (#33579) - **src**: disallow JS execution inside FreeEnvironment (Anna Henningsen) (#33874) - **src**: remove \_third\_party\_main support (Anna Henningsen) (#33971) - **src**: remove deprecated node debug command (James M Snell) (#33648) - **src**: remove unused CancelPendingDelayedTasks (Anna Henningsen) (#32859) - **stream**: try to wait for flush to complete before 'finish' (Robert Nagy) (#34314) - **stream**: cleanup and fix Readable.wrap (Robert Nagy) (#34204) - **stream**: add promises version to utility functions (rickyes) (#33991) - **stream**: fix writable.end callback behavior (Robert Nagy) (#34101) - **stream**: construct (Robert Nagy) (#29656) - **stream**: write should throw on unknown encoding (Robert Nagy) (#33075) - **stream**: fix \_final and 'prefinish' timing (Robert Nagy) (#32780) - **stream**: simplify Transform stream implementation (Robert Nagy) (#32763) - **stream**: use callback to properly propagate error (Robert Nagy) (#29179) - **test**: update tests after increasing typed array size to 4GB (Kim-Anh Tran) (#35415) - **test**: fix tests for npm 7.0.0 (Myles Borins) (#35631) - **test**: fix test suite to work with npm 7 (Myles Borins) (#35474) - **test**: update WPT harness and tests (Michaël Zasso) (#33770) - **timers**: introduce timers/promises (James M Snell) (#33950) - **tools**: disable x86 safe exception handlers in V8 (Michaël Zasso) (#35415) - **tools**: update V8 gypfiles for 8.6 (Ujjwal Sharma) (#35415) - **tools**: update V8 gypfiles for 8.5 (Ujjwal Sharma) (#35415) - **url**: file URL path normalization (Daijiro Wachi) (#35477) - **url**: verify domain is not empty after "ToASCII" (Michaël Zasso) (#33770) - **url**: remove U+0000 case in the fragment state (Michaël Zasso) (#33770) - **url**: remove gopher from special schemes (Michaël Zasso) (#33325) - **url**: forbid lt and gt in url host code point (Yash Ladha) (#33328) - **util**: change default value of `maxStringLength` to 10000 (unknown) (#32744) - **wasi**: drop --experimental-wasm-bigint requirement (Colin Ihrig) (#35415) - **win, child_process**: sanitize env variables (Bartosz Sosnowski) (#35210) - **worker**: make MessageEvent class more Web-compatible (Anna Henningsen) (#35496) - **worker**: set trackUnmanagedFds to true by default (Anna Henningsen) (#34394) - **worker**: rename error code to be more accurate (Anna Henningsen) (#33872) PR-URL: #35014

Document deprecation of rmdir/recursive permissive functionality PR-URL: nodejs#35579 Refs: nodejs#35562 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Antoine du Hamel <[email protected]> Reviewed-By: Michaël Zasso <[email protected]>

This is a follow up to nodejs#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: nodejs#35562 Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Ben Coe <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

nodejs-github-bot added the fs Issues and PRs related to the fs subsystem / file system. label Oct 8, 2020

bcoe added semver-major PRs that contain breaking changes and should be released in the next major version. notable-change PRs with changes that should be highlighted in changelogs. labels Oct 8, 2020

bcoe requested review from mhdawson, bengl, mcollina and Trott October 8, 2020 21:27

BridgeAR reviewed Oct 8, 2020

View reviewed changes

bcoe reviewed Oct 8, 2020

View reviewed changes

lib/internal/fs/utils.js Outdated Show resolved Hide resolved

test/parallel/test-fs-rmdir-recursive.js Outdated Show resolved Hide resolved

bcoe reviewed Oct 9, 2020

View reviewed changes

lib/fs.js Show resolved Hide resolved

mcollina approved these changes Oct 9, 2020

View reviewed changes

Trott reviewed Oct 9, 2020

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

Trott reviewed Oct 9, 2020

View reviewed changes

bcoe mentioned this pull request Oct 9, 2020

doc: document rmdir/recursive deprecation #35579

Closed

4 tasks

Trott reviewed Oct 10, 2020

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

Trott added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2020

Trott approved these changes Oct 10, 2020

View reviewed changes

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 10, 2020

bcoe reviewed Oct 12, 2020

View reviewed changes

doc/api/deprecations.md Outdated Show resolved Hide resolved

iansu force-pushed the rmdir-recursive-deprecation-warning branch from 440cbed to c1fc25a Compare October 12, 2020 21:12

bcoe added 2 commits October 13, 2020 10:29

chore: address merge conflicts

94bb5ea

chore: address merge issues

2fd90bd

bcoe force-pushed the rmdir-recursive-deprecation-warning branch from c23ce69 to 2fd90bd Compare October 13, 2020 17:30

mhdawson approved these changes Oct 13, 2020

View reviewed changes

Trott reviewed Oct 14, 2020

View reviewed changes

lib/internal/fs/utils.js Outdated Show resolved Hide resolved

Update deprecation message

5978379

bcoe approved these changes Oct 14, 2020

View reviewed changes

bcoe added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 15, 2020

github-actions bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Oct 15, 2020

This comment has been minimized.

Sign in to view

bcoe closed this Oct 15, 2020

aduh95 mentioned this pull request Feb 3, 2021

fs: remove permissive rmdir recursive #37216

Merged

targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Sep 5, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fs: deprecation warning on recursive rmdir #35562

fs: deprecation warning on recursive rmdir #35562

iansu commented Oct 8, 2020

BridgeAR Oct 8, 2020

iansu Oct 8, 2020

bcoe left a comment

mcollina left a comment

aduh95 commented Oct 9, 2020

bcoe commented Oct 9, 2020

Trott left a comment

bcoe commented Oct 9, 2020

Trott commented Oct 9, 2020

bcoe commented Oct 10, 2020

Trott commented Oct 10, 2020

nodejs-github-bot commented Oct 10, 2020

Trott commented Oct 10, 2020

nodejs-github-bot commented Oct 10, 2020

nodejs-github-bot commented Oct 13, 2020 •

edited by bcoe

Loading

bcoe commented Oct 13, 2020

mhdawson left a comment

bcoe commented Oct 13, 2020

Trott commented Oct 14, 2020

Trott commented Oct 14, 2020

nodejs-github-bot commented Oct 14, 2020

bcoe commented Oct 14, 2020

This comment has been minimized.

bcoe commented Oct 15, 2020

fs: deprecation warning on recursive rmdir #35562

fs: deprecation warning on recursive rmdir #35562

Conversation

iansu commented Oct 8, 2020

Checklist

BridgeAR Oct 8, 2020

Choose a reason for hiding this comment

iansu Oct 8, 2020

Choose a reason for hiding this comment

bcoe left a comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

aduh95 commented Oct 9, 2020

bcoe commented Oct 9, 2020

Trott left a comment

Choose a reason for hiding this comment

bcoe commented Oct 9, 2020

Trott commented Oct 9, 2020

bcoe commented Oct 10, 2020

Trott commented Oct 10, 2020

nodejs-github-bot commented Oct 10, 2020

Trott commented Oct 10, 2020

nodejs-github-bot commented Oct 10, 2020

nodejs-github-bot commented Oct 13, 2020 • edited by bcoe Loading

bcoe commented Oct 13, 2020

mhdawson left a comment

Choose a reason for hiding this comment

bcoe commented Oct 13, 2020

Trott commented Oct 14, 2020

Trott commented Oct 14, 2020

nodejs-github-bot commented Oct 14, 2020

bcoe commented Oct 14, 2020

This comment has been minimized.

bcoe commented Oct 15, 2020

nodejs-github-bot commented Oct 13, 2020 •

edited by bcoe

Loading