Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fs: deprecate never throw behaviour in fs.existsSync #55753

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Ceres6
Copy link
Contributor

@Ceres6 Ceres6 commented Nov 6, 2024

This PR attempts to deprecate the behaviour of returning false on argument validation in fs.existsSync

cc @joyeecheung @BridgeAR

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Nov 6, 2024
lib/fs.js Outdated
@@ -286,6 +286,7 @@ function existsSync(path) {
try {
path = getValidatedPath(path);
} catch {
util.deprecate(() => {}, 'never throw on invalid arguments for fs.existsSync is deprecated', 'DEP0186');
Copy link
Member

Choose a reason for hiding this comment

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

This means that this is not a documentation-only deprecation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed, I think this Runtime, isn't it?

@Ceres6 Ceres6 force-pushed the chore/deprecate-fs-existSync-never-throw branch from ac72356 to c3d38fb Compare November 6, 2024 17:02
Copy link

codecov bot commented Nov 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (1aa7135) to head (2e2d342).
Report is 42 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #55753   +/-   ##
=======================================
  Coverage   88.40%   88.40%           
=======================================
  Files         654      654           
  Lines      187747   187747           
  Branches    36127    36118    -9     
=======================================
+ Hits       165972   165977    +5     
- Misses      15009    15014    +5     
+ Partials     6766     6756   -10     
Files with missing lines Coverage Δ
lib/fs.js 98.14% <100.00%> (+<0.01%) ⬆️

... and 24 files with indirect coverage changes

@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Nov 14, 2024
doc/api/deprecations.md Outdated Show resolved Hide resolved
description: Runtime deprecation.
-->

Type: Runtime
Copy link
Contributor

Choose a reason for hiding this comment

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

We should first land it as doc-only

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I open a different PR for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

You can, we'll need two PRs anyway, you can either keep this one for the runtime and open a new one for the doc-only, or convert this one to be doc-only and open another one for the runtime deprecation later.

Co-authored-by: Antoine du Hamel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants