fix(ext/node): throw ERR_INVALID_ARG_VALUE when rmdir gets options.recursive#33565
Conversation
…cursive Match Node's `lib/fs.js` rmdir/rmdirSync — the `recursive` option was deprecated in Node and removed; passing it now throws `ERR_INVALID_ARG_VALUE` with "is no longer supported". We previously emitted a deprecation warning and silently routed through the recursive remove path, hiding the divergence. Also drop the now-unused `rmdirRecursive` helper and stale imports (`emitRecursiveRmdirWarning`, `RmOptions`, `ERR_FS_RMDIR_ENOTDIR`). Enables `parallel/test-fs-rmdir-recursive-error.js`.
nathanwhitbot
left a comment
There was a problem hiding this comment.
Faithful port of Node's lib/fs.js L1115–1161:
rmdir(async): same order as Node —typeof options === 'function'→
options?.recursive !== undefinedthrow →validateFunction(Node uses
makeCallback) →getValidatedPath…→validateRmdirOptions→ op. ✓rmdirSync: also matches Node's order — path validation first, then the
recursive-not-supported throw, thenvalidateRmdirOptions. ✓- The
options.recursive === falsepath still throws (because the check is
!== undefined), matching Node. ✓ - Cleanups (
rmdirRecursivehelper + theemitRecursiveRmdirWarning,
RmOptions,ERR_FS_RMDIR_ENOTDIRimports) are correct — those were the
only callers and they're gone now.
fs.promises.rmdir going through promisify(rmdir) works for this
specifically because the throw happens inside the Promise executor that
util.promisify builds, so it surfaces as a rejected Promise — same shape
as Node's hand-rolled async function rmdir. Worth noting for future
divergences (fs.promises.rmdir is async upstream, not promisified), but
no behavior gap today.
LGTM.
fibibot
left a comment
There was a problem hiding this comment.
The substantive change is right — Node deprecated and removed the recursive option on fs.rmdir / fs.rmdirSync, and the verbatim message + code (ERR_INVALID_ARG_VALUE, 'is no longer supported') match lib/fs.js upstream. The validation order (recursive-throw before path/callback validation in async, after path validation in sync) also matches Node. The rmdirRecursive helper and the now-unused imports are correctly removed.
But CI is failing on two concrete issues that block merge:
-
tests/unit_node/_fs/_fs_rmdir_test.tshas two tests that exercise the now-removed recursive path. "ASYNC: removing non-empty folder" and "SYNC: removing non-empty folder" both callrmdir(dir, { recursive: true }, ...)/rmdirSync(dir, { recursive: true })and assert success. Those need to be migrated torm/rmSync(which is the supported replacement) or deleted, since the behavior they test is the one this PR is removing. Inline detail. -
tools/lint_plugins/no_deno_api_in_polyfills.tsEXPECTED_VIOLATIONSentry forext/node/polyfills/fs.tsneeds to drop from 53 to 51 — removingrmdirRecursiveand the recursive branch inrmdirSynceliminates twoDeno.remove(...)calls. The lint plugin checksactual === expectedand is now off by -2.
Both are mechanical fixes but the PR will stay red until they land. Sibling tests the body lists pass because they don't use recursive: true; the _fs_rmdir_test.ts cases are the only ones that hit the removed path.
- Migrate the two non-empty-folder removal tests off rmdir({recursive})
(which now throws ERR_INVALID_ARG_VALUE) to rm/rmSync({recursive}),
per fibibot's review on denoland#33565.
- Update tools/lint_plugins/no_deno_api_in_polyfills.ts EXPECTED count
for fs.ts from 53 to 51 — removing rmdirRecursive + the recursive
branch in rmdirSync drops two Deno.remove(...) calls.
Summary
Node deprecated and then removed the
recursiveoption onfs.rmdir/fs.rmdirSync; passing it now throwsERR_INVALID_ARG_VALUEwith the message "is no longer supported" (seelib/fs.js). Deno was still emitting a deprecation warning and silently routing through the recursive remove path, which hid the divergence —rm/rmSyncare the supported replacement.This patch:
ERR_INVALID_ARG_VALUEfromfs.rmdir,fs.rmdirSync, and (via promisify)fs.promises.rmdirwhenoptions.recursiveis provided.rmdirRecursivehelper plus theemitRecursiveRmdirWarning,RmOptions, andERR_FS_RMDIR_ENOTDIRimports it depended on.Enables
parallel/test-fs-rmdir-recursive-error.jsin the Node compat suite.Test plan
cargo test --test node_compat -- test-fs-rmdir-recursive-error(was failing on main, passes here)cargo build --bin denocleantools/format.jsandtools/lint.js --jscleantest-fs-rm,test-fs-rmSync-special-char,test-fs-rmdir-throws-not-found,test-fs-rmdir-throws-on-file,test-fs-rmdir-type-check.