Skip to content

src: namespace config file flags - #57170

Closed
marco-ippolito wants to merge 2 commits into
nodejs:mainfrom
marco-ippolito:namespacing-config-file
Closed

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

Conversation

@marco-ippolito

Copy link
Copy Markdown
Member

follow-up: #57016 (comment)

@nodejs-github-botnodejs-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

Copy link
Copy Markdown
MemberAuthor

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

@codecov

codecovBot commented Feb 21, 2025

Copy link
Copy Markdown

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 linesPatch %Lines
src/node_config_file.cc95.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 linesCoverage Δ
lib/internal/options.js100.00% <100.00%> (ø)
src/node_config_file.cc71.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.

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

@marco-ippolito

Copy link
Copy Markdown
MemberAuthor

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

@marco-ippolitomarco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 21, 2025
@nodejs-github-bot

This comment was marked as outdated.

@GeoffreyBooth

Copy link
Copy Markdown
Member

🤔 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.

@GeoffreyBoothGeoffreyBooth left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Comment threaddoc/api/cli.md Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

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

@marco-ippolito

Copy link
Copy Markdown
MemberAuthor

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

@nodejs-github-bot

This comment was marked as outdated.

Comment threadlib/internal/options.js Outdated
Comment threadsrc/node_config_file.cc Outdated
@marco-ippolitomarco-ippolito added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2025
@github-actionsgithub-actionsBot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 22, 2025
@nodejs-github-bot

nodejs-github-bot commented Feb 22, 2025

Copy link
Copy Markdown
Collaborator

RafaelGSS pushed a commit that referenced this pull request May 1, 2025
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
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
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
PR-URL: #57170
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
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
PR-URL: nodejs#57170
Backport-PR-URL: nodejs#57958
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@aduh95aduh95 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
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
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
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
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
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
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
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
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
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
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
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
PR-URL: #57170
Backport-PR-URL: #57958
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
@ghostghost mentioned this pull request Jun 8, 2025
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backported-to-v22.xPRs backported to the v22.x-staging branch.c++Issues and PRs that require attention from people who are familiar with C++.commit-queue-rebaseAdd this label to allow the Commit Queue to land a PR in several commits.configIssues or PRs related to the config subsystemneeds-ciPRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@marco-ippolito@nodejs-github-bot@GeoffreyBooth@RafaelGSS@ShogunPanda@jasnell@anonrig@robpalme@BridgeAR@aduh95