Skip to content

fs: remove recursive option from rmdir - #57784

Closed
LiviaMedeiros wants to merge 3 commits into
nodejs:mainfrom
LiviaMedeiros:fs-eol-deprecate-rmdir-recursive
Closed

fs: remove recursive option from rmdir#57784
LiviaMedeiros wants to merge 3 commits into
nodejs:mainfrom
LiviaMedeiros:fs-eol-deprecate-rmdir-recursive

Conversation

@LiviaMedeiros

Copy link
Copy Markdown
Member

This moves DEP0147: fs.rmdir(path, { recursive: true }) to EOL.

The recursive option was runtime deprecated since v16.0.0 in favor of rm() function. Maybe it is time to remove it.

cc @nodejs/fs because fsIssues and PRs related to the fs subsystem / file system.
cc @nodejs/tsc because semver-majorPRs that contain breaking changes and should be released in the next major version.
cc @addaleax because of concerns in the runtime deprecation PR #37302 (comment)

@LiviaMedeirosLiviaMedeiros added fs Issues and PRs related to the fs subsystem / file system. semver-major PRs that contain breaking changes and should be released in the next major version. deprecations Issues and PRs related to deprecations. labels Apr 7, 2025
@nodejs-github-botnodejs-github-bot added the needs-ci PRs that need a full CI run. label Apr 7, 2025
@LiviaMedeirosLiviaMedeiros added the needs-citgm PRs that need a CITGM CI run. label Apr 7, 2025

@mcollinamcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@codecov

codecovBot commented Apr 7, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.25%. Comparing base (33f6e1e) to head (f9034bc).
Report is 559 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #57784 +/- ##
========================================
Coverage 90.24% 90.25% ========================================
Files 630 630 Lines 185245 185136 -109 Branches 36299 36286 -13 ========================================
- Hits 167173 167090 -83 + Misses 11018 10988 -30 - Partials 7054 7058 +4 
Files with missing linesCoverage Δ
lib/fs.js98.25% <100.00%> (-0.02%)⬇️
lib/internal/fs/promises.js98.22% <ø> (-0.02%)⬇️
lib/internal/fs/utils.js99.68% <100.00%> (-0.01%)⬇️

... and 39 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nodejs-github-bot

nodejs-github-bot commented Apr 7, 2025

Copy link
Copy Markdown
Collaborator

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@mcollina

Copy link
Copy Markdown
Member

There seem to be a lot of CITGM failures compared to main.

@LiviaMedeiros

Copy link
Copy Markdown
MemberAuthor

Closed in favour of #58616

@jasnell

Copy link
Copy Markdown
Member

oh! I had completely missed this @LiviaMedeiros ! Sorry about that :-*(

@LiviaMedeiros

Copy link
Copy Markdown
MemberAuthor

No worries; as long as the change lands, am happy with it.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecationsIssues and PRs related to deprecations.fsIssues and PRs related to the fs subsystem / file system.needs-ciPRs that need a full CI run.needs-citgmPRs that need a CITGM CI run.semver-majorPRs that contain breaking changes and should be released in the next major version.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants

@LiviaMedeiros@nodejs-github-bot@mcollina@jasnell@lpinca@MoLow@jazelly@RaisinTen@bjohansebas