Skip to content

fs: runtime deprecate fs.F_OK, fs.R_OK, fs.W_OK, fs.X_OK#49686

Merged
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
LiviaMedeiros:fs-runtime-deprecate-moisting-constants
Nov 14, 2024
Merged

fs: runtime deprecate fs.F_OK, fs.R_OK, fs.W_OK, fs.X_OK#49686
nodejs-github-bot merged 5 commits into
nodejs:mainfrom
LiviaMedeiros:fs-runtime-deprecate-moisting-constants

Conversation

@LiviaMedeiros

@LiviaMedeiros LiviaMedeiros commented Sep 17, 2023

Copy link
Copy Markdown
Member

Follow-up from: #49683 (doc-only deprecation)

Opening this ahead of time to point out that the getters here are non-enumerable.

cc @nodejs/fs

@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 Sep 17, 2023
@aduh95 aduh95 added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 18, 2023
@LiviaMedeiros LiviaMedeiros added the blocked PRs that are blocked by other issues or PRs. label Sep 20, 2023
@LiviaMedeiros LiviaMedeiros marked this pull request as ready for review September 20, 2023 07:15
@LiviaMedeiros LiviaMedeiros added deprecations Issues and PRs related to deprecations. and removed blocked PRs that are blocked by other issues or PRs. labels Sep 25, 2023
@LiviaMedeiros LiviaMedeiros force-pushed the fs-runtime-deprecate-moisting-constants branch from 7a9498e to 956fcf1 Compare September 25, 2023 15:22
@richardlau

Copy link
Copy Markdown
Member

Given that we've only just doc-deprecated these #49683 (not in a release yet) I think it would be premature to runtime deprecate in Node.js 21. I'd have no objections to runtime deprecation in Node.js 22.

@LiviaMedeiros

Copy link
Copy Markdown
Member Author

Sure; converting back to draft until v21.x is released.

The main point of opening it now was to see if there are any objections or suggestions to the implementation.
If we keep the getters with enumerable: true, the deprecation warning will be indirectly triggered by child_process; I don't know if there is any better workaround.

@LiviaMedeiros LiviaMedeiros marked this pull request as draft September 25, 2023 17:50
@LiviaMedeiros LiviaMedeiros marked this pull request as ready for review November 18, 2023 19:10
@LiviaMedeiros LiviaMedeiros force-pushed the fs-runtime-deprecate-moisting-constants branch 2 times, most recently from be1b1c3 to c865ced Compare February 26, 2024 10:49
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 26, 2024
@nodejs-github-bot

This comment was marked as outdated.

@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@juanarbol

Copy link
Copy Markdown
Member

@LiviaMedeiros this may needs rebase, this is a good timing for v24.x I think (?)

@LiviaMedeiros LiviaMedeiros force-pushed the fs-runtime-deprecate-moisting-constants branch from c865ced to d5b465c Compare November 3, 2024 05:28
@LiviaMedeiros LiviaMedeiros added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Nov 3, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

@LiviaMedeiros

Copy link
Copy Markdown
Member Author

Rebased on main.

cc @nodejs/tsc since it's semver-major PRs that contain breaking changes and should be released in the next major version.

@codecov

codecov Bot commented Nov 3, 2024

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.40%. Comparing base (c2ff449) to head (7000c66).
Report is 74 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #49686      +/-   ##
==========================================
- Coverage   88.41%   88.40%   -0.02%     
==========================================
  Files         654      654              
  Lines      187594   187696     +102     
  Branches    36090    36114      +24     
==========================================
+ Hits       165870   165936      +66     
- Misses      14980    14996      +16     
- Partials     6744     6764      +20     
Files with missing lines Coverage Δ
lib/fs.js 98.26% <100.00%> (+0.11%) ⬆️

... and 34 files with indirect coverage changes

@aduh95

aduh95 commented Nov 3, 2024

Copy link
Copy Markdown
Contributor

@LiviaMedeiros LiviaMedeiros added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 4, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Nov 14, 2024
@nodejs-github-bot nodejs-github-bot merged commit b02cd41 into nodejs:main Nov 14, 2024
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in b02cd41

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

Labels

author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. 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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants