src: namespace config file flags by marco-ippolito · Pull Request #57170 · nodejs/node · GitHub

🤔 no code-owners... wonder who I should ping, or create a team

Codecov Report

Attention: Patch coverage is 96.66667% with 2 lines in your changes missing coverage. Please review.

Project coverage is 90.35%. Comparing base (8fc919d) to head (c9fe4d6).
Report is 565 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_config_file.cc	95.65%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #57170   +/-   ##
=======================================
  Coverage   90.35%   90.35%           
=======================================
  Files         629      629           
  Lines      184308   184394   +86     
  Branches    36015    36027   +12     
=======================================
+ Hits       166532   166617   +85     
+ Misses      10909    10902    -7     
- Partials     6867     6875    +8

Files with missing lines	Coverage Δ
lib/internal/options.js	100.00% <100.00%> (ø)
src/node_config_file.cc	71.96% <95.65%> (+8.94%)	⬆️

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

I would rather not use the namespace and just specify a specific file that we use instead of letting the user define the filename.

I would rather not use the namespace and just specify a specific file that we use instead of letting the user define the filename.

I added the default file in #57171, about namespacing I think its a necessary evil, otherwise we will regret it if the feature grows

🤔 no code-owners... wonder who I should ping, or create a team

I think we need a @nodejs/cli team or something, to cover this feature and --env-file. Please add me if you create one.

There was discussion in old issues about supporting various environments, like top-level fields such as dev or localhost or prod etc. I don't have an opinion as to how multiple environments or multiple config files should be handled, but I feel like this is something to be worked out now as we're discussing namespacing. If the answer is that we're just not going to provide any explicit support for merging config files or supporting multiple environments within one config file, that's fine, but I think we should have that discussion.

Can we support all of experimental-transform-types, experimental_transform_types and experimentalTransformTypes for each flag? Just as currently the CLI supports either of the first two.

I'm ok with the second, not sure how feasible is the third one. I think we can defer to a followup

Given this is just a sample of how to use the config file to persist flags, would it be better to stick to non-experimental flags?

This example maximizes hiding of the dependency on experimental features, e.g. by disabling the warnings too. That's fine if you are experienced enough to understand the implications.

Whilst it's certainly possible to use configs for this purpose, having it as the shining model example seems risky IMO and may be construed as encouragement to use this pattern.

will do in the followup PR #57171 since this is ready to land and we have added the not-land-on-v23

I dont think we should support different environments in the same file, users can use the dev.config.json and the prod.config.json

CI: https://ci.nodejs.org/job/node-test-pull-request/65372/ 💛

PR-URL: #57170 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

PR-URL: nodejs#57170 Backport-PR-URL: nodejs#57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Feb 21, 2025

marco-ippolito requested review from BridgeAR, bengl, jasnell and targos February 21, 2025 14:54

marco-ippolito force-pushed the namespacing-config-file branch from e2a1d89 to 453c339 Compare February 21, 2025 14:59

jasnell approved these changes Feb 21, 2025

View reviewed changes

marco-ippolito mentioned this pull request Feb 21, 2025

src: add config file support #57016

Closed

BridgeAR reviewed Feb 21, 2025

View reviewed changes

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2025

This comment was marked as outdated.

Sign in to view

GeoffreyBooth reviewed Feb 21, 2025

View reviewed changes

This comment was marked as outdated.

Sign in to view

anonrig reviewed Feb 22, 2025

View reviewed changes

Comment thread lib/internal/options.js Outdated

Comment thread src/node_config_file.cc Outdated

marco-ippolito added 2 commits February 22, 2025 10:32

src: namespace config file flags

8f0a66f

test: add more coverage to node_config_file

c9fe4d6

marco-ippolito force-pushed the namespacing-config-file branch from 83e5231 to c9fe4d6 Compare February 22, 2025 09:32

marco-ippolito requested a review from anonrig February 22, 2025 09:33

marco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2025

marco-ippolito mentioned this pull request Feb 22, 2025

src: set default config as node.config.json #57171

Merged

ShogunPanda approved these changes Feb 24, 2025

View reviewed changes

RafaelGSS pushed a commit that referenced this pull request May 1, 2025

src: namespace config file flags

c49692e

PR-URL: #57170 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

RafaelGSS pushed a commit that referenced this pull request May 1, 2025

test: add more coverage to node_config_file

76a1757

PR-URL: #57170 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

RafaelGSS pushed a commit that referenced this pull request May 2, 2025

src: namespace config file flags

04fb99d

PR-URL: #57170 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

RafaelGSS pushed a commit that referenced this pull request May 2, 2025

test: add more coverage to node_config_file

06a58f4

PR-URL: #57170 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 mentioned this pull request May 6, 2025

[v22.x backport] src: add config file support #57958

Merged

aduh95 pushed a commit to marco-ippolito/node that referenced this pull request May 6, 2025

src: namespace config file flags

143064b

PR-URL: nodejs#57170 Backport-PR-URL: nodejs#57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 pushed a commit to marco-ippolito/node that referenced this pull request May 6, 2025

test: add more coverage to node_config_file

a6cff20

PR-URL: nodejs#57170 Backport-PR-URL: nodejs#57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 added backported-to-v22.x PRs backported to the v22.x-staging branch. and removed backport-open-v22.x Indicate that the PR has an open backport backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels May 6, 2025

aduh95 pushed a commit that referenced this pull request May 6, 2025

src: namespace config file flags

0545e1b

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 pushed a commit that referenced this pull request May 6, 2025

test: add more coverage to node_config_file

3004682

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

RafaelGSS pushed a commit that referenced this pull request May 14, 2025

src: namespace config file flags

9ff58d5

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

RafaelGSS pushed a commit that referenced this pull request May 14, 2025

test: add more coverage to node_config_file

033127c

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 pushed a commit that referenced this pull request May 16, 2025

src: namespace config file flags

287a0c1

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 pushed a commit that referenced this pull request May 16, 2025

test: add more coverage to node_config_file

5758187

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 pushed a commit that referenced this pull request May 17, 2025

src: namespace config file flags

3612f2d

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 pushed a commit that referenced this pull request May 17, 2025

test: add more coverage to node_config_file

c8c3872

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 pushed a commit that referenced this pull request May 18, 2025

src: namespace config file flags

ec4201d

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 pushed a commit that referenced this pull request May 18, 2025

test: add more coverage to node_config_file

e24076d

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

github-actions Bot mentioned this pull request May 19, 2025

2025-05-21, Version 22.16.0 'Jod' (LTS) #58388

Merged

aduh95 pushed a commit that referenced this pull request May 19, 2025

src: namespace config file flags

ccee741

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

aduh95 pushed a commit that referenced this pull request May 19, 2025

test: add more coverage to node_config_file

e90583b

PR-URL: #57170 Backport-PR-URL: #57958 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Paolo Insogna <paolo@cowtech.it>

ghost mentioned this pull request Jun 8, 2025

### Notable Changes autofix-ci/action#28

Closed

src: namespace config file flags by marco-ippolito · Pull Request #57170 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: namespace config file flags - #57170

src: namespace config file flags#57170marco-ippolito wants to merge 2 commits intonodejs:mainnodejs/node:mainfrom marco-ippolito:namespacing-config-filemarco-ippolito/node:namespacing-config-fileCopy head branch name to clipboard

Conversation

marco-ippolito commented Feb 21, 2025

Uh oh!

marco-ippolito commented Feb 21, 2025

Uh oh!

codecov Bot commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

BridgeAR left a comment

Choose a reason for hiding this comment

Uh oh!

marco-ippolito commented Feb 21, 2025

Uh oh!

This comment was marked as outdated.

GeoffreyBooth commented Feb 21, 2025

Uh oh!

GeoffreyBooth left a comment

Choose a reason for hiding this comment

Uh oh!

GeoffreyBooth Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

marco-ippolito Feb 22, 2025

Choose a reason for hiding this comment

Uh oh!

robpalme Feb 23, 2025

Choose a reason for hiding this comment

Uh oh!

marco-ippolito Feb 24, 2025

Choose a reason for hiding this comment

Uh oh!

marco-ippolito commented Feb 21, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Uh oh!

nodejs-github-bot commented Feb 22, 2025 • edited by jasnell Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

src: namespace config file flags#57170
marco-ippolito wants to merge 2 commits into
nodejs:mainfrom
marco-ippolito:namespacing-config-file

codecov Bot commented Feb 21, 2025 •
edited
Loading

nodejs-github-bot commented Feb 22, 2025 •
edited by jasnell
Loading