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

src: add config file support #57016

Closed
wants to merge 2 commits into from

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Feb 12, 2025

Refs: #53787

Introducing node.json

node --experimental-config-file=noderc.json file.js

Why a config file?

With the introduction of test runner, sea, and other feature that require a lot of flags, a json config flag would improve by a lot the developer experience and increase adoption.
Example:

node --experimental-config-file=noderc.json --test --experimental-test-coverage
{
 "test-coverage-lines": 80,
 "test-coverage-branches": 60
}

Current solutions

  1. CLI Flags
  2. NODE_OPTIONS
  3. --env-file with NODE_OPTIONS

All of these solutions dont have a good developer experience.

Why JSON?

Simply because we already support json parsing (simdjson) and its kinda the default in the js ecosystem.

Do we have to support all the configurations possible?

NO

Not all flag need to be supported by the config file, and not all flags CAN be supported.
The config file will only support flags that can be passed through NODE_OPTIONS:
https://nodejs.org/api/cli.html#node_optionsoptions

How does it work?

This is an early development, it work similarly to the --env-file flag. It will parse the provided file with --experimental-config-file and add to NODE_OPTIONS the recognized flags. Unrecognized flags will be ignored.

Versioning

json_schema!
When we release a new version, we also release a json schema file.

{
  "$schema": "https://nodejs.org/dist/v20.18.3/docs/node_config_json_schema.json",
}

The users can has autocompletion and validation of their config file specific to the version they are using.
Since the config file mirrors the features that the current version has, it would be impossible to provide semver major backwards compatibility (immagine a feature gets removed we also have to remove it from the config file).

Examples in the ecosystem

Security

Just like .env files, Node.js entrusts the user to provide a secure and valid configuration.

@nodejs/tsc for visibility since its a big feature

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp
  • @nodejs/startup

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Feb 12, 2025
@anonrig
Copy link
Member

anonrig commented Feb 12, 2025

I took a somewhat similar approach last year. Happy to talk in private if you need any help. Ref: anonrig#4

@richardlau richardlau added the semver-minor PRs that contain new features and should be released in the next minor version. label Feb 12, 2025
@marco-ippolito marco-ippolito force-pushed the feat/config-file branch 2 times, most recently from 272828d to 497298b Compare February 12, 2025 21:53
@marco-ippolito marco-ippolito force-pushed the feat/config-file branch 4 times, most recently from 2e99027 to 8028f5a Compare February 12, 2025 22:22
@AugustinMauroy
Copy link
Member

Should this configuration file include experimental options?
I don't think so, it should just allow config to be used from feature mark to stable.

@AugustinMauroy
Copy link
Member

And if this configuration file could support the version of node. It could issue a warning if the wrong version is used for the project.
But that would require node to be able to resolve semver ranges.
And I think that would really add a degree of complexity to the file.

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Feb 13, 2025

I don't see the downside of supporting experimental features. Users can validate with the json schema we provide for the version they are using (you can read in the docs how)

@AugustinMauroy
Copy link
Member

I think that enabling feature experimental support will increase their adoption, and so in the event of a breaking change, it will impact more users.

@marco-ippolito marco-ippolito marked this pull request as ready for review February 16, 2025 12:07
Copy link

codecov bot commented Feb 16, 2025

Codecov Report

Attention: Patch coverage is 72.48062% with 71 lines in your changes missing coverage. Please review.

Project coverage is 90.35%. Comparing base (3b0fce1) to head (a7ee706).
Report is 10 commits behind head on main.

Files with missing lines Patch % Lines
src/node_config_file.cc 63.02% 33 Missing and 11 partials ⚠️
src/node_options.cc 62.50% 14 Missing and 10 partials ⚠️
src/node.cc 83.33% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #57016      +/-   ##
==========================================
- Coverage   90.38%   90.35%   -0.03%     
==========================================
  Files         628      629       +1     
  Lines      184007   184265     +258     
  Branches    35952    36008      +56     
==========================================
+ Hits       166314   166499     +185     
- Misses      10869    10907      +38     
- Partials     6824     6859      +35     
Files with missing lines Coverage Δ
lib/internal/options.js 100.00% <100.00%> (ø)
lib/internal/process/pre_execution.js 93.31% <100.00%> (+1.65%) ⬆️
src/node_errors.h 85.00% <ø> (ø)
src/node_options.h 98.32% <ø> (ø)
src/node.cc 73.96% <83.33%> (+0.38%) ⬆️
src/node_options.cc 85.32% <62.50%> (-2.05%) ⬇️
src/node_config_file.cc 63.02% <63.02%> (ø)

... and 21 files with indirect coverage changes

@jasnell
Copy link
Member

jasnell commented Feb 16, 2025

Historically I've been fairly anti-config file but I actually think I've come around to the idea. I do think, however, that we need to be careful about making things too confusing for users with some options being passed as command line flags, others being passed as environment variables, and now others possibly being passed by config file. Ideally these mechanisms would integrate with each other such that we can add a configuration option once and it just works (much like we can do today with adding a command line flag and indicate whether it can be used in NODE_OPTIONS or not). It would even be ideal for us to either be able to add an option to the JSON schema and automatically have the code generated in the runtime to support the option or add the option programmatically and provide a way for node.js to generate the json schema dynamically from that. Either way, I've come around to supporting moving in this direction (even if while kicking and screaming).

src/node_rc.cc Outdated
namespace node {

ConfigReader::ParseResult ConfigReader::ParseExperimentalObject(
simdjson::ondemand::object* experimental_object,
Copy link
Member

Choose a reason for hiding this comment

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

@lemire ... I'm curious, does simdjson support jsonc? (JSON with comments). Would it be difficult to add support for jsonc if not? If we're going to use json as a configuration format, then supporting comments is ideal. (note that both deno and bun support comments in their respective formats)

@marco-ippolito
Copy link
Member Author

I had to rebase after #57142 😭

@marco-ippolito marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 20, 2025
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@marco-ippolito marco-ippolito added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. labels Feb 20, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 20, 2025
@nodejs-github-bot
Copy link
Collaborator

Landed in 5ab7c4c...772c609

nodejs-github-bot pushed a commit that referenced this pull request Feb 20, 2025
PR-URL: #57016
Refs: #53787
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
nodejs-github-bot pushed a commit that referenced this pull request Feb 20, 2025
PR-URL: #57016
Refs: #53787
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
@bengl
Copy link
Member

bengl commented Feb 21, 2025

Would it be possible in a future commit to allow for (but maybe not require) options to be namespaced in a "node_options" property?

e.g.

{
  "node_options": {
    "test-coverage-lines": 80,
    "test-coverage-branches": 60
  }
}

This would allow for re-using existing JSON files currently used for other kinds of config.

@marco-ippolito
Copy link
Member Author

Unless we decide to support other use cases in addition to flags, imho it does not make sense to namespace it.

@bengl
Copy link
Member

bengl commented Feb 21, 2025

If it's namespaced, I can just use package.json and not need yet another file. I understand that some folks don't like packing extra info in there, but we've already done that with type.

EDIT: After I posted, I realized that as-is I can just use top-level properties in a package.json. Properties aren't likely to collide with other package.json properties, but namespacing them would ensure no collisions.

@targos
Copy link
Member

targos commented Feb 21, 2025

Moving to nodeOptions (camelCase) SGTM, just to make room for future options that can be more ergonomic than a 1-1 mapping to CLI flags.

@BridgeAR
Copy link
Member

package.json is already overboarded in my opinion and it is a cleare separation to have Node.js configurations in a file that is actually also meant to be local for each user. I could have local settings that I don't want to share with others. So separating the files is good in my opinion.

@bengl
Copy link
Member

bengl commented Feb 21, 2025

@BridgeAR Nothing stops anyone from using separate files if they want to. This PR requires that the file be specified, and namespacing makes it easier me to use the same file for more than one purpose, to avoid having tons of config files all over the root of my project. Having separate files for the reasons you indicated also totally makes sense, but I think either approach should be possible because not all use cases are predictable.

Also, as @targos mentioned, namespacing adds a layer of future-proofing. We don't know what folks will generally want in the future (both as developers of the project and as users), and namespacing makes non-breaking changes more possible.

@BridgeAR
Copy link
Member

BridgeAR commented Feb 21, 2025

Should we consider removing the need for the flag? I would personally prefer a fixed file name that is just checked for in the root folder than having it dynamically configured by users. That way more people would likely pick it up seeing the file better and it requires the user to think about it less / it's easier to use.
That way there is also no discussion around namespacing it. It would just be that one file.

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Feb 21, 2025

Let's move the discussion to #57170

@GeoffreyBooth
Copy link
Member

I didn't notice this PR being opened, much less landing. If there had been a comment posted on #53787 about this, everyone who was following that issue would've been aware of this PR while there was still time to review it. Some of these post-landing concerns such as about namespacing or default filenames could've been addressed before landing.

That said, I don't have any objections to this PR as is, so I'm not asking for it to be reverted or anything like that. Perhaps we should mark it as dont-land-on-v23.x until we can sort through some of the issues that people have brought up above?

@marco-ippolito marco-ippolito added the dont-land-on-v23.x PRs that should not land on the v23.x-staging branch and should not be released in v23.x. label Feb 21, 2025

```json
{
"$schema": "https://nodejs.org/dist/REPLACEME/docs/node_config_json_schema.json",
Copy link
Member

@styfle styfle Feb 22, 2025

Choose a reason for hiding this comment

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

Can we remove the duplicate json?

node_config_schema.json

Or better yet, use hyphens

node-config-schema.json

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea

acidiney pushed a commit to acidiney/node that referenced this pull request Feb 23, 2025
PR-URL: nodejs#57016
Refs: nodejs#53787
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
acidiney pushed a commit to acidiney/node that referenced this pull request Feb 23, 2025
PR-URL: nodejs#57016
Refs: nodejs#53787
Reviewed-By: Yagiz Nizipli <[email protected]>
Reviewed-By: Tierney Cyren <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Paolo Insogna <[email protected]>
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++. commit-queue-rebase Add this label to allow the Commit Queue to land a PR in several commits. dont-land-on-v23.x PRs that should not land on the v23.x-staging branch and should not be released in v23.x. lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. notable-change PRs with changes that should be highlighted in changelogs. semver-minor PRs that contain new features and should be released in the next minor version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.