Skip to content

fs: unexpose internal constants #58327

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

Merged
merged 1 commit into from
May 16, 2025

Conversation

legendecas
Copy link
Member

EXTENSIONLESS_FORMAT_JAVASCRIPT and EXTENSIONLESS_FORMAT_WASM are
only used internally through binding getFormatOfExtensionlessFile.
They should not be exposed publicly.

Add a test to cover public fs.constants to avoid unexpected keys.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. typings labels May 14, 2025
`EXTENSIONLESS_FORMAT_JAVASCRIPT` and `EXTENSIONLESS_FORMAT_WASM` are
only used internally through binding `getFormatOfExtensionlessFile`.
They should not be exposed publicly.
@legendecas legendecas force-pushed the constants/internal branch from 2d1466f to e228d6f Compare May 14, 2025 13:05
Copy link

codecov bot commented May 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.23%. Comparing base (b197355) to head (e228d6f).
Report is 20 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #58327      +/-   ##
==========================================
- Coverage   90.23%   90.23%   -0.01%     
==========================================
  Files         633      633              
  Lines      186818   186827       +9     
  Branches    36668    36668              
==========================================
+ Hits       168578   168584       +6     
+ Misses      11036    11035       -1     
- Partials     7204     7208       +4     
Files with missing lines Coverage Δ
lib/internal/modules/esm/formats.js 98.64% <100.00%> (ø)
src/node_constants.cc 99.71% <100.00%> (+<0.01%) ⬆️

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

@legendecas legendecas added the request-ci Add this label to start a Jenkins CI on a PR. label May 15, 2025
@github-actions github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels May 15, 2025
Copy link
Contributor

Failed to start CI
   ⚠  Commits were pushed since the last approving review:
   ⚠  - fs: unexpose internal constants
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/15045856908

@legendecas legendecas removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label May 15, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@ljharb
Copy link
Member

ljharb commented May 15, 2025

is this a breaking change, then?

@aduh95
Copy link
Contributor

aduh95 commented May 15, 2025

is this a breaking change, then?

I think breaking an undocumented feature is not considered a semver-major change as long as it doesn't affect the ecosystem. We can decide to not land it on LTS just to be sure – another approach that would limit the number of conflicts would be to land a first iteration moving them to that new internal binding, but still expose them on fs.constants, and we can remove it in a follow-up atomic commit.

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@legendecas
Copy link
Member Author

legendecas commented May 16, 2025

These two undocumented constants are not used in any public APIs. Their values are just 0 and 1. I don't think the two constants can be used in any meaningful way.

@juanarbol juanarbol added commit-queue Add this label to land a pull request using GitHub Actions. author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed needs-ci PRs that need a full CI run. labels May 16, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label May 16, 2025
@nodejs-github-bot nodejs-github-bot merged commit a436f7b into nodejs:main May 16, 2025
64 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in a436f7b

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. c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. typings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants