Skip to content

fs: only show deprecation warning when error code matches - #56549

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
aduh95:fs-existsSync-deprecation-warning
Feb 28, 2025
Merged

fs: only show deprecation warning when error code matches#56549
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
aduh95:fs-existsSync-deprecation-warning

Conversation

@aduh95

Copy link
Copy Markdown
Contributor

The deprecation is only for invalid types, it could be confusing the warning show up for an unrelated error.

Refs: #55753

@aduh95aduh95 added dont-land-on-v18.x dont-land-on-v22.x PRs that should not land on the v22.x-staging branch and should not be released in v22.x. labels Jan 10, 2025
@nodejs-github-botnodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Jan 10, 2025

@BridgeARBridgeAR 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.

I believe all Node.js errors should trigger the warning and only programmer errors in Node.js code should not be caught. Is that correct? We don't yet have such utility method to check that. We could however check for any error code existence. It should not exist on regular programmer errors.

@codecov

codecovBot commented Jan 10, 2025

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.16%. Comparing base (529b56e) to head (6f62176).
Report is 394 commits behind head on main.

Additional details and impacted files
@@ Coverage Diff @@## main #56549 +/- ##
=======================================
Coverage 89.16% 89.16% =======================================
Files 662 662 Lines 191746 191746 Branches 36902 36901 -1 =======================================
+ Hits 170970 170972 +2 
Misses 13632 13632 + Partials 7144 7142 -2 
Files with missing linesCoverage Δ
lib/fs.js98.30% <100.00%> (ø)

... and 24 files with indirect coverage changes

@aduh95

aduh95 commented Feb 26, 2025

Copy link
Copy Markdown
ContributorAuthor

I believe all Node.js errors should trigger the warning and only programmer errors in Node.js code should not be caught. Is that correct? We don't yet have such utility method to check that. We could however check for any error code existence. It should not exist on regular programmer errors.

IMO that's a separate issue (IIUC you're complaining about the fact we blindly catch any error, instead of rethrowing non-Node.js ones). What this PR is addressing is that the warning should only appear when there's a ARG_TYPE error (as that's what has been deprecated).

@aduh95aduh95 added the review wanted PRs that need reviews. label Feb 26, 2025
@aduh95aduh95 added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Feb 26, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@bjohansebasbjohansebas 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

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@aduh95aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 28, 2025
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 28, 2025
@nodejs-github-bot
nodejs-github-bot merged commit 48f381d into nodejs:mainFeb 28, 2025
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 48f381d

@aduh95
aduh95 deleted the fs-existsSync-deprecation-warning branch February 28, 2025 10:57
RafaelGSS pushed a commit that referenced this pull request Apr 1, 2025
PR-URL: #56549
Refs: #55753
Reviewed-By: Jacob Smith <jacob@frende.me>
RafaelGSS pushed a commit that referenced this pull request May 1, 2025
PR-URL: #56549
Refs: #55753
Reviewed-By: Jacob Smith <jacob@frende.me>
RafaelGSS pushed a commit that referenced this pull request May 2, 2025
PR-URL: #56549
Refs: #55753
Reviewed-By: Jacob Smith <jacob@frende.me>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.dont-land-on-v22.xPRs that should not land on the v22.x-staging branch and should not be released in v22.x.fsIssues and PRs related to the fs subsystem / file system.needs-ciPRs that need a full CI run.review wantedPRs that need reviews.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@aduh95@nodejs-github-bot@JakobJingleheimer@BridgeAR@bjohansebas