-
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: set default config as node.config.json #57171
base: main
Are you sure you want to change the base?
Conversation
Hmm, I'm a bit concerned about naming collisions with existing uses of |
Maybe noderc.json? That should not have collisions (I HOPE 😆) |
In any case, I don't think it should be unflagged from the first release. |
it's not unflagged, I'm not sure what are you referring to |
Okay then, I guess I didn't read the commit message correctly |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57171 +/- ##
==========================================
- Coverage 90.35% 90.27% -0.08%
==========================================
Files 629 630 +1
Lines 184424 184643 +219
Branches 36034 36138 +104
==========================================
+ Hits 166633 166691 +58
- Misses 10913 11011 +98
- Partials 6878 6941 +63
|
When searching I was only able to find examples of |
Best options are |
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.
How does this play with file permissions?
I added a test, also will need a rebase after #57170 lands. There are 6k results for I wish we could just take it 🥺 Otherwise:
|
IMO, |
f1b6d67
to
70b3289
Compare
70b3289
to
e00ade9
Compare
|
c0373f2
to
6456729
Compare
434360e
to
54b272c
Compare
@@ -976,6 +977,18 @@ unknown keys or keys that cannot used in `NODE_OPTIONS`. | |||
Node.js will not sanitize or perform validation on the user-provided configuration, | |||
so **NEVER** use untrusted configuration files. | |||
|
|||
### `--experimental-default-config-file` |
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.
When the day comes that we load node.config.json
without needing any flags, I assume we’ll need a way to disable that behavior and not just as a temporary “while still experimental” measure, because I assume that some people will consider it a security hazard that we might load configuration implicitly. (Though we already do so via NODE_OPTIONS
, I guess, so if you think I’m wrong about this assumption I’m very open to be persuaded that I’m wrong.) So when this feature is stable and if we do need a permanent way to prevent the automatic loading, I think the options are either --no-default-config-file
or --disable-default-config-file
. We have a bunch of --no-*
flags and also a bunch of --disable-*
flags, and I’m not sure if there’s any reasoning behind one versus the other.
Personally I think disable
makes more sense for this particular feature from an English language perspective of “disable the loading of the default config file”, as opposed to “no default config file” being a bit ambiguous (am I telling it not to load a default config file, or that Node should error if there is no default config file, or something else?). But I defer to the crowd on this one.
54b272c
to
3e6cfd1
Compare
This PR changes the current behavior:
Will automatically look for a
node.config.json
file (I dont have strong opinion on the default)If a
--experimental-config-file
is provided it will use that: