-
Notifications
You must be signed in to change notification settings - Fork 30.7k
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
src: add config file support #57016
Conversation
Review requested:
|
I took a somewhat similar approach last year. Happy to talk in private if you need any help. Ref: anonrig#4 |
5d7ff50
to
3f46529
Compare
272828d
to
497298b
Compare
2e99027
to
8028f5a
Compare
8028f5a
to
67192b9
Compare
Should this configuration file include experimental options? |
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. |
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) |
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. |
67192b9
to
fe60554
Compare
Codecov ReportAttention: Patch coverage is
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
|
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 |
src/node_rc.cc
Outdated
namespace node { | ||
|
||
ConfigReader::ParseResult ConfigReader::ParseExperimentalObject( | ||
simdjson::ondemand::object* experimental_object, |
There was a problem hiding this comment.
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)
This comment was marked as outdated.
This comment was marked as outdated.
Maybe some builds (without intl or crypto) don't expose some flags 🤔 thats why comparing the json schema with the available flags fails |
Yes, for example some options depend on OpenSSL being present: Lines 1105 to 1111 in f6ce486
|
04d3ecb
to
a56b4a5
Compare
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
This comment was marked as outdated.
This comment was marked as outdated.
a56b4a5
to
a7ee706
Compare
I had to rebase after #57142 😭 |
Landed in 5ab7c4c...772c609 |
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]>
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]>
Would it be possible in a future commit to allow for (but maybe not require) options to be namespaced in a 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. |
Unless we decide to support other use cases in addition to flags, imho it does not make sense to namespace it. |
Refs: #53787
Introducing node.json
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:
Current solutions
--env-file
with NODE_OPTIONSAll 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 toNODE_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.
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