src: parse dotenv with the rest of the options

[avivkeller]

Closes#54385
Fixes#54255
Ref #54232

---

I don't think this breaks anything. IIUC, the Dotenv parser was setup before everything else so that it triggered before V8 initialized, however, even it's setup after the options are parsed, V8 still hasn't been initialized, so it should be fine.

---

This fixes all currently known dotenv edge cases, as it doesn't need a custom parser.

Such as:

node script.js --env-file .env
node --env-file-ABCD .env
node -- --env-file .env
node --invalid --env-file .env # this would've errored, but the env file is still parsed

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[codecov]

Codecov Report

Attention: Patch coverage is 89.23077% with 7 lines in your changes missing coverage. Please review.

Project coverage is 88.40%. Comparing base (9f9069d) to head (6395703).
Report is 1186 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node.cc	77.77%	4 Missing and 2 partials ⚠️
src/node_options.cc	95.83%	1 Missing ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #54913 +/- ##
==========================================
+ Coverage 88.39% 88.40% +0.01% 
==========================================
Files 652 654 +2 Lines 186777 187649 +872 Branches 36039 36093 +54 ==========================================
+ Hits 165109 165899 +790 - Misses 14917 14994 +77 - Partials 6751 6756 +5 

Files with missing lines	Coverage Δ
src/node_dotenv.cc	93.66% <100.00%> (+1.13%)	⬆️
src/node_dotenv.h	100.00% <ø> (ø)
src/node_options-inl.h	81.10% <100.00%> (+0.29%)	⬆️
src/node_options.h	98.28% <100.00%> (+0.05%)	⬆️
src/node_options.cc	87.86% <95.83%> (+0.45%)	⬆️
src/node.cc	74.08% <77.77%> (+0.33%)	⬆️

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

[targos]

If it fixes a lot of things, shouldn't it add a lot of tests?

[avivkeller]

Tests have been added + rebased.

[targos]

The way the test is done is not very robust: if the error message changes in the future, it will be wrong but still pass. It would be better to assert the actual error

[avivkeller]

The failed GitHub tests are flakes, could someone start a CI? I don't have the permissions to start one without approvals

[avivkeller]

I have to incorporate #53060

[BoscoDomingo]

I have to incorporate #53060

FYI @anonrig had a plan to use string_views instead of strings for the file paths (I couldn't make it work), so you may want to check with him to avoid stepping on each other's work 👍🏻

[anonrig]

Can you rebase your branch with main, with the recent changes, it is extremely hard to review.

[avivkeller]

I'm currently facing the problem of: The files aren't parsed in the same order they were supplied. It's loading all of the optionals, then all of the normals. Any ideas?

[BoscoDomingo]

I'm currently facing the problem of: The files aren't parsed in the same order they were supplied. It's loading all of the optionals, then all of the normals. Any ideas?

Well you are grabbing them all by creating vectors and referencing those via cli_options, then calling process_files on the optional files before the required ones. I'm on my phone so I can't check, but I would assume that could be the reason.

You're likely respecting argument order inside each vector, it's just you're segregating them by optionality first

[avivkeller]

You're likely respecting argument order inside each vector, it's just you're segregating them by optionality first

Exactly... I just don't know how to fix that.

[BoscoDomingo]

You're likely respecting argument order inside each vector, it's just you're segregating them by optionality first

Exactly... I just don't know how to fix that.

It's one thing that Dotenv::GetDataFromArgs did. It may simply be incompatible with the goal of this PR? Not knowledgeable enough with the code to assert that with confidence tbh. However, my first guess would be to store the position each argument so you can later on zip both lists together. Again, no idea if possible but a potential solution

e.g.

node --env-file=1.env --some-other-arg --env-file=2.env --inspect --env-file-if-exists=3.env
cli_options->env_files = {
0: "1.env",
2: "2.env"
}
cli_options->optional_env_files = {
4: "3.env"
}

[avivkeller]

I fixed the issue by iterating over the arguments. It's kind of a mix between what was done previously.

[avivkeller]

I'm not a expert CPP developer, so LMK if I made a mistake.

[avivkeller]

As shocking as it may seem, all Github failures are unrelated flakes.

---

@anonrig is this better to what you had in mind?

[xduugu]

First of all, thanks a lot for still working on a proper fix for the env file option issues, @redyetidev.

I'm currently facing the problem of: The files aren't parsed in the same order they were supplied. It's loading all of the optionals, then all of the normals. Any ideas?

I saw that you already fixed it, but maybe it could be solved much easier:

From my understanding, the order of --env-file and --env-file-if-exists options is only relevant, if there are variables that occur in multiple env files. Because an env file overwrites variables with the same name from env files read before, it makes no sense to load the "optional" env file before the "required" one, because the latter will always overwrite the former one (please correct me if I miss a use case). So the only order that makes sense, is loading the "required" env file before the "optional" one, because the optional env file is only read if it exists.

My proposed solution is, first reading all the "required" env files in the order they appear in the option list, then all "optional" ones, and adding a note about the env file processing order to the documentation.

[avivkeller]

IMHO the current setup makes more sense, that is, parse in the order the options where passed.

[avivkeller]

I've modified the implementation to use a new DetailedOption parsing method, which gets both the option value and the option flag, This allows for the program to accurately determine which option index and which option type to parse.

[avivkeller]

@anonrig are you still blocking? I think I've resolved all your concerns.

As for the string copying, I believe it's needed, as other functions are expecting a string.

[avivkeller]

PTAL @nodejs/cpp-reviewers

[avivkeller]

Any other concerns, @lemire?

[lemire]

@redyetidev I don't have any concern, but I'd like to hear what @anonrig has to say about this PR. He is still asking for changes which I consider blocking.

One thing is certain: this has been opened in September. We should do something with it.