close
Skip to content

src: make node.config.json throw at unknown fields#62992

Open
marco-ippolito wants to merge 1 commit intonodejs:mainfrom
marco-ippolito:sanitize-config-file
Open

src: make node.config.json throw at unknown fields#62992
marco-ippolito wants to merge 1 commit intonodejs:mainfrom
marco-ippolito:sanitize-config-file

Conversation

@marco-ippolito
Copy link
Copy Markdown
Member

The documentation is correct but the implementation was not

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

Review requested:

  • @nodejs/config

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run. labels Apr 27, 2026
Signed-off-by: Marco Ippolito <marcoippolito54@gmail.com>
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 89.66%. Comparing base (2428030) to head (8923655).
⚠️ Report is 70 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #62992      +/-   ##
==========================================
+ Coverage   89.63%   89.66%   +0.03%     
==========================================
  Files         706      707       +1     
  Lines      219219   219512     +293     
  Branches    42004    42088      +84     
==========================================
+ Hits       196499   196832     +333     
+ Misses      14622    14581      -41     
- Partials     8098     8099       +1     
Files with missing lines Coverage Δ
src/node_config_file.cc 83.96% <100.00%> (+0.23%) ⬆️

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

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 27, 2026
@ljharb
Copy link
Copy Markdown
Member

ljharb commented Apr 27, 2026

won't this mean that you can't make a config file that supports multiple versions of node and gracefully uses features from the newer ones when available?

@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 27, 2026
@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@marco-ippolito
Copy link
Copy Markdown
Member Author

won't this mean that you can't make a config file that supports multiple versions of node and gracefully uses features from the newer ones when available?

Yes but also means if you mispell a configuration you will know. I prefer correctness over convenience.

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

@ljharb
Copy link
Copy Markdown
Member

ljharb commented Apr 28, 2026

It's not just about convenience, though - it's about being able to support multiple node versions at one time, which also makes upgrading easier (and not supporting that makes upgrading harder).

I very much prioritize correctness, but having a closed config every time is highly likely to hold back the ecosystem.

@marco-ippolito
Copy link
Copy Markdown
Member Author

marco-ippolito commented Apr 28, 2026

The support for multiple versions of node in the same configuration was never planned and should be discouraged. The $schema needs to match with version being used. I think a possible solution to this problem would be support an array of configurations and node can pick the right one based on version. This plus the ability to extend (like tsconfig)

@nodejs-github-bot
Copy link
Copy Markdown
Collaborator

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

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. config Issues or PRs related to the config subsystem needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants