src: extend `--env-file` to also accept sections #58782

dario-piotrowicz · 2025-06-21T21:49:08Z

Fixes: #58729

nodejs-github-bot · 2025-06-21T21:49:13Z

Review requested:

@nodejs/config
@nodejs/startup

codecov · 2025-06-21T22:49:36Z

Codecov Report

Attention: Patch coverage is 88.79310% with 13 lines in your changes missing coverage. Please review.

Project coverage is 90.08%. Comparing base (e679e38) to head (5af1b31).
Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_dotenv.cc	87.32%	4 Missing and 5 partials ⚠️
src/node_util.cc	76.92%	0 Missing and 3 partials ⚠️
src/node.cc	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58782      +/-   ##
==========================================
- Coverage   90.08%   90.08%   -0.01%     
==========================================
  Files         640      640              
  Lines      188316   188370      +54     
  Branches    36922    36936      +14     
==========================================
+ Hits       169650   169687      +37     
+ Misses      11406    11404       -2     
- Partials     7260     7279      +19

Files with missing lines	Coverage Δ
lib/internal/process/per_thread.js	`99.46% <100.00%> (+<0.01%)`	⬆️
lib/util.js	`97.79% <100.00%> (+0.02%)`	⬆️
src/node_dotenv.h	`100.00% <ø> (ø)`
src/node_process_methods.cc	`88.09% <100.00%> (+0.23%)`	⬆️
src/node.cc	`74.86% <50.00%> (+0.03%)`	⬆️
src/node_util.cc	`81.79% <76.92%> (+<0.01%)`	⬆️
src/node_dotenv.cc	`85.65% <87.32%> (+1.04%)`	⬆️

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

src/node_process_methods.cc

Make sure that `--env-file` can accept sections, also extend the `parseEnv` and `process.loadEnvFile` APIs to accept sections as well

vitaly-t · 2025-06-22T22:58:42Z

Based on the issue discussion, I believe we are only short of testing variables with dots in the name:

one.two.three = some value

[section]
four.five = another value

At the moment we only test section names with dots.

The rest looks good! 👍

lib/internal/process/per_thread.js

lib/util.js

lib/internal/process/per_thread.js

anonrig · 2025-06-22T23:07:06Z

src/node_dotenv.cc

@@ -26,6 +27,33 @@ std::vector<Dotenv::env_file_data> Dotenv::GetDataFromArgs(
           arg.starts_with("--env-file-if-exists=");
  };

+  const auto get_sections = [](const std::string& path) {


I recommend always using std::string_view since you'll always know when you'll copy (by calling std::string(val))

Suggested change

const auto get_sections = [](const std::string& path) {

const auto get_sections = [](std::string_view path) {

anonrig · 2025-06-22T23:10:29Z

src/node_dotenv.cc

@@ -154,6 +215,33 @@ void Dotenv::ParseContent(const std::string_view input) {
      continue;
    }

+    if (content.front() == '[') {


Can you add a comment to here explaining why you're checking for [?

He is checking for the start of a section, I believe - [section_name]

Yes, but a year from now, a contributor might have some hard time understanding the reasoning.

comment added 🙂

anonrig · 2025-06-22T23:11:12Z

src/node_dotenv.cc

@@ -303,7 +391,8 @@ void Dotenv::ParseContent(const std::string_view input) {
  }
 }

-Dotenv::ParseResult Dotenv::ParsePath(const std::string_view path) {
+Dotenv::ParseResult Dotenv::ParsePath(const std::string_view path,
+                                      const std::set<std::string> sections) {


If you don't add &, you'll always copy.

Suggested change

const std::set<std::string> sections) {

const std::set<std::string>& sections) {

anonrig · 2025-06-22T23:11:43Z

src/node_process_methods.cc

@@ -590,21 +590,42 @@ static void Execve(const FunctionCallbackInfo<Value>& args) {
 }
 #endif

+struct IterateSectionsData {
+  Isolate* isolate;
+  std::set<std::string>* sections;


This is considered unsafe. Why do you need to have a pointer to a set?

anonrig · 2025-06-22T23:13:20Z

src/node_process_methods.cc

+  ToNamespacedPath(env, &path_value);
+  auto path = path_value.ToString();
+
+  CHECK(args[1]->IsArray());


If you make this optional, you don't need to allocate a std::set every time this function is called.

anonrig · 2025-06-22T23:14:24Z

src/node_process_methods.cc


  THROW_IF_INSUFFICIENT_PERMISSIONS(
      env, permission::PermissionScope::kFileSystemRead, path);

  Dotenv dotenv{};

-  switch (dotenv.ParsePath(path)) {
+  switch (dotenv.ParsePath(path, sections)) {


This copies the function.

If you want to avoid copy you should make it set&, if you want to move the ownership to the function since you don't need it inside this function, you should set function parameter to set&& and pass the argument as std::move(sections)

validate sections only when present in options object

update jsdoc types for options Co-authored-by: Yagiz Nizipli <[email protected]>

dario-piotrowicz · 2025-06-22T23:53:39Z

The rest looks good! 👍

Thanks 😄

Based on the issue discussion, I believe we are only short of testing variables with dots in the name:
one.two.three = some value

[section]
four.five = another value
At the moment we only test section names with dots.

As I mentioned in the discussion I believe that variable key testing is unrelated to the sections support addition (and that more testing is missing not just only around dots) so I don't see a strong need for adding it here and I would much rather do that in its own dedicated PR, do you disagree? 🙂

fix comment typo

add code comment explaining the `[` check

fix cpp formatting

anonrig · 2025-06-23T00:48:25Z

I agree that dot based variable overriding is unrelated and probably an overkill feature to implement.

vitaly-t · 2025-06-23T00:57:07Z

I agree that dot based variable overriding is unrelated and probably an overkill feature to implement.

Dots in in the variables names work currently (I tested it in v24.2.0), so it is not a feature, and extra tests would be just to cover that we do not break that.

dario-piotrowicz requested a review from anonrig June 21, 2025 21:49

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 Jun 21, 2025

dario-piotrowicz mentioned this pull request Jun 21, 2025

Extend --env-file with support for INI sections #58729

Open

dario-piotrowicz force-pushed the dario/58729/env-ini-sections branch from ff88ca1 to c1edd49 Compare June 21, 2025 21:59

This comment was marked as resolved.

Sign in to view

jasnell reviewed Jun 22, 2025

View reviewed changes

src/node_process_methods.cc Outdated Show resolved Hide resolved

dario-piotrowicz force-pushed the dario/58729/env-ini-sections branch from c1edd49 to ee7e6c3 Compare June 22, 2025 17:41

dario-piotrowicz requested a review from jasnell June 22, 2025 17:43

src: extend dot-env to also handle sections

d6e8866

Make sure that `--env-file` can accept sections, also extend the `parseEnv` and `process.loadEnvFile` APIs to accept sections as well

dario-piotrowicz force-pushed the dario/58729/env-ini-sections branch from ee7e6c3 to d6e8866 Compare June 22, 2025 17:56

anonrig reviewed Jun 22, 2025

View reviewed changes

dario-piotrowicz and others added 2 commits June 23, 2025 00:44

fixup! src: extend dot-env to also handle sections

e5e6528

validate sections only when present in options object

fixup! src: extend dot-env to also handle sections

967e671

update jsdoc types for options Co-authored-by: Yagiz Nizipli <[email protected]>

dario-piotrowicz added 3 commits June 23, 2025 00:55

fixup! src: extend dot-env to also handle sections

67b49d0

fix comment typo

fixup! src: extend dot-env to also handle sections

1be2a2f

add code comment explaining the `[` check

fixup! src: extend dot-env to also handle sections

5af1b31

fix cpp formatting

	const auto get_sections = [](const std::string& path) {
	const auto get_sections = [](std::string_view path) {

	const std::set<std::string> sections) {
	const std::set<std::string>& sections) {

Uh oh!

src: extend --env-file to also accept sections #58782

Are you sure you want to change the base?

src: extend --env-file to also accept sections #58782

Uh oh!

Conversation

dario-piotrowicz commented Jun 21, 2025

Uh oh!

nodejs-github-bot commented Jun 21, 2025

Uh oh!

This comment was marked as resolved.

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

Uh oh!

Codecov Report

Uh oh!

Uh oh!

vitaly-t commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dario-piotrowicz commented Jun 22, 2025

Uh oh!

anonrig commented Jun 23, 2025

Uh oh!

vitaly-t commented Jun 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

src: extend `--env-file` to also accept sections #58782

src: extend `--env-file` to also accept sections #58782

codecov bot commented Jun 21, 2025 •

edited

Loading

vitaly-t commented Jun 22, 2025 •

edited

Loading

vitaly-t commented Jun 23, 2025 •

edited

Loading