src: parse dotenv with the rest of the options

[RedYetiDev]

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 will error, but the env file is still parsed

[nodejs-github-bot]

Review requested:

• @nodejs/startup

[codecov]

Codecov Report

Attention: Patch coverage is 87.80488% with 5 lines in your changes missing coverage. Please review.

Project coverage is 88.24%. Comparing base (3c4ef34) to head (3decd15).
Report is 96 commits behind head on main.

Files with missing lines	Patch %	Lines
src/node_dotenv.cc	83.33%	3 Missing and 1 partial ⚠️
src/node.cc	94.11%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #54913      +/-   ##
==========================================
+ Coverage   88.06%   88.24%   +0.17%     
==========================================
  Files         652      652              
  Lines      183544   183902     +358     
  Branches    35861    35864       +3     
==========================================
+ Hits       161644   162282     +638     
+ Misses      15151    14912     -239     
+ Partials     6749     6708      -41     

Files with missing lines	Coverage Δ
src/node_dotenv.h	100.00% <ø> (ø)
src/node_options.cc	88.04% <ø> (-0.02%)	⬇️
src/node_options.h	98.21% <ø> (ø)
src/node.cc	74.42% <94.11%> (+0.56%)	⬆️
src/node_dotenv.cc	91.56% <83.33%> (-0.97%)	⬇️

... and 109 files with indirect coverage changes

[targos]

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

[RedYetiDev]

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

[RedYetiDev]

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

[RedYetiDev]

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.

[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?

[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

[RedYetiDev]

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"
}

[RedYetiDev]

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

[anonrig]

This removed implementation doesn't make any unnecessary copy, but your new implementation does.

[RedYetiDev]

It's not unneccessery. It needs to be an std::string for ProcessGlobalArgsInternal

[RedYetiDev]

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