Skip to content

Conversation

jasnell
Copy link
Member

@jasnell jasnell commented Jun 11, 2025

The processReadResult and readSyncRecursive methods on the fs.Dir class were mistakenly exposed as public APIs. These methods are intended only for internal use. They are now runtime deprecated and will be removed.

There is an extremely slim possibility that this change could be breaking in that if someone out there happened to be monkeypatching these then the approach taken in this PR would break that. A github code search revealed no such cases and I believe it's unlikely enough that we shouldn't need to worry about it.

The `processReadResult` and `readSyncRecursive` methods on the
`fs.Dir` class were mistakenly exposed as public APIs. These
methods are intended only for internal use. They are now
runtime deprecated and will be removed.
@jasnell jasnell 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 Jun 11, 2025
@nodejs-github-bot nodejs-github-bot added the needs-ci PRs that need a full CI run. label Jun 11, 2025
@jasnell jasnell requested a review from LiviaMedeiros June 11, 2025 15:49
@jasnell
Copy link
Member Author

jasnell commented Jun 11, 2025

Alternative to #58672

Copy link

codecov bot commented Jun 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.14%. Comparing base (977b5ac) to head (ce1bce7).
Report is 15 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58679      +/-   ##
==========================================
- Coverage   90.15%   90.14%   -0.01%     
==========================================
  Files         636      636              
  Lines      188061   188078      +17     
  Branches    36899    36903       +4     
==========================================
+ Hits       169537   169552      +15     
- Misses      11278    11280       +2     
  Partials     7246     7246              
Files with missing lines Coverage Δ
lib/internal/fs/dir.js 94.80% <100.00%> (+0.23%) ⬆️

... and 27 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.

Copy link
Member

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

I'm totes fine with either

  • Landing this as is (runtime deprecation in v25.x, EOL in 26.x+)
  • Landing this as semver-patch, with or without backporting (runtime deprecation is v24.x-, EOL in 25.x+)
  • Landing #58672 as semver-major (immediate EOL in 25.x)
  • Landing #58672 as is, with or without backporting (immediate EOL in 24.x-)


### DEP0198: `fs.Dir.prototype.readSyncRecursive()` and `fs.Dir.prototype.processReadResult`

<!--
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<!--
<!-- YAML

<!--
changes:
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/00000
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
pr-url: https://github.com/nodejs/node/pull/00000
pr-url: https://github.com/nodejs/node/pull/58679


Type: Runtime

The `fs.Dir.prototype.readSyncRecursive()` and `fs.Dir.prototype.processReadResult`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
The `fs.Dir.prototype.readSyncRecursive()` and `fs.Dir.prototype.processReadResult`
The `fs.Dir.prototype.readSyncRecursive()` and `fs.Dir.prototype.processReadResult()`

or

Suggested change
The `fs.Dir.prototype.readSyncRecursive()` and `fs.Dir.prototype.processReadResult`
The `fs.Dir.prototype.readSyncRecursive` and `fs.Dir.prototype.processReadResult`


The [`util.types.isNativeError`][] API is deprecated. Please use [`Error.isError`][] instead.

### DEP0198: `fs.Dir.prototype.readSyncRecursive()` and `fs.Dir.prototype.processReadResult`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
### DEP0198: `fs.Dir.prototype.readSyncRecursive()` and `fs.Dir.prototype.processReadResult`
### DEP0198: `fs.Dir.prototype.readSyncRecursive()` and `fs.Dir.prototype.processReadResult()`

or

Suggested change
### DEP0198: `fs.Dir.prototype.readSyncRecursive()` and `fs.Dir.prototype.processReadResult`
### DEP0198: `fs.Dir.prototype.readSyncRecursive` and `fs.Dir.prototype.processReadResult`

@aduh95
Copy link
Contributor

aduh95 commented Jun 11, 2025

If it's undocumented and we find no use in the ecosystem, I don't think we need a deprecation, we can directly remove it in a semver-major IMO

@jasnell
Copy link
Member Author

jasnell commented Jun 12, 2025

Let's get some other opinions from @nodejs/tsc on it... if there are no objections to handling it without a deprecation then ok

@jasnell jasnell added the tsc-agenda Issues and PRs to discuss during the meetings of the TSC. label Jun 12, 2025
@anonrig
Copy link
Member

anonrig commented Jun 12, 2025

I'm ok with landing without a deprecation, and just removing/hiding it.

@panva
Copy link
Member

panva commented Jun 13, 2025

If it's undocumented and we find no use in the ecosystem, I don't think we need a deprecation, we can directly remove it in a semver-major IMO

I concur. Landing #58672 as semver-major PRs that contain breaking changes and should be released in the next major version. is sufficient.

@jasnell
Copy link
Member Author

jasnell commented Jun 14, 2025

Closing in favor of #58672

@jasnell jasnell closed this Jun 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deprecations Issues and PRs related to deprecations. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. semver-major PRs that contain breaking changes and should be released in the next major version. tsc-agenda Issues and PRs to discuss during the meetings of the TSC.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants