src: add support to override local env variables option

[IlyasShabi]

Following https://github.com/nodejs/node/issues/49148 This PR aims to add support to override local env variables with variables from defined env files.

• [x] using CLI
• [x] using loadEnvFile

[nodejs-github-bot]

Review requested:

• [ ] @nodejs/startup

[IlyasShabi]

@anonrig when reading the issue about using .env files, I noticed a discussion about overriding local environment settings: https://github.com/nodejs/node/issues/49148#issuecomment-1704891766 https://github.com/nodejs/node/issues/49148#issuecomment-2025490811 I created this PR to add support for overriding, similar to what dotenv offers here Is there a specific process I should follow before creating a PR?

[Trott]

I'm not sure who to ping for more eyes/comments on this, but I'll try @nodejs/tooling.

[GeoffreyBooth]

Is there a specific process I should follow before creating a PR?

There isn’t, but generally I encourage people to open an issue proposing a new feature before going ahead and coding the new feature. That way you get preliminary feedback before you implement, which might change the way you implement it and save you time as compared to doing revisions (like renaming this flag, for example). Or if the feedback is very negative, it might save you from wasting your time implementing a feature that won’t land. If nothing else, it separates the feedback related to the idea of the feature from the feedback related to the implementation of the feature; the issue can contain the former and the PR would get the latter.

For this feature in particular, since it’s not on the list of anticipated enhancements to --env-file, I think it would have been good to open an issue dedicated to this to discuss specifically whether this is something that we want. My initial impression is that if it’s something that we thought would be good to add, it would’ve been on the TODO list; so I’m already skeptical as to whether this is a good idea or not. I don’t think the question has been asked directly, though, so maybe it’s fine; I think it’s worth having that conversation, in more detail than the few comments you found within other threads. I think in general we’re not trying to put dotenv out of business; our scope is not to match every feature they offer, so just because dotenv does something doesn’t mean that we’ll automatically copy it.

[IlyasShabi]

@GeoffreyBooth Thank you for explaining this. I was inspired by the discussions to contribute. I see why it's important to discuss new features in an issue first. This can help shape the feature better and save time. In the future, I'll start with an issue to gather feedback before I begin coding.

[github-actions[bot]]

The https://github.com/nodejs/node/labels/notable-change label has been added by @anonrig.

Please suggest a text for the release notes if you'd like to include a more detailed summary, then proceed to update the PR description with the text or a link to the notable change suggested text comment. Otherwise, the commit will be placed in the Other Notable Changes section.

[joyeecheung]

currently these examples use a very mixed bag of import/require.

From what I can tell the docs are just using

1. direct access of process from the context + require('node:assert') in CJS examples
2. import 'node:process' + import 'node:assert' in ESM examples

I am not sure if you noticed but that has been the way to write the examples in both ways to allow toggling between CJS/ESM style in the doc (e.g. see https://nodejs.org/docs/latest/api/fs.html#promise-example for how the toggling works in the web page).

[bnb]

@joyeecheung yep, I'm aware - I've written a handful of docs that do that/added missing CJS or ESM to examples where only one is present.

Here's examples of how the docs here are inconsistent:

import assert from 'node:assert';
import process from 'node:process';

const assert = require('node:assert');
const { loadEnvFile } = require('node:process');
loadEnvFile('.env', { override: true });

import assert from 'node:assert';
import process, { loadEnvFile } from 'node:process';
loadEnvFile('.env', { override: true });
assert.strictEqual(process.env.API_KEY, 'env-custom-key');

import'ing process and then { loadEnvFile } is IMO a very weird pattern? I've never seen it before at least. The flipping between implicit process vs the one in ESM kinda is awful ngl - is this required? I guess I didn't know that was how you'd do this.

There's kinda a lot in a small space, so I figured pointing issues out broadly as the few categories that occur multiple times would be helpful but if it's not I'm happy to comment every problem individually.

[joyeecheung]

I see, there are some inconsistencies in import styles though I think in this code base in general, style preferences should not be blocking unless the linter complains. I’d suggest adding linter rules to forbid it or following it up with doc change PR.

[GeoffreyBooth]

disagree that we shouldn’t block on docs being inconsistent but I don’t think this is the venue to hash that out

I don’t think it’s a blocking concern for this PR. It’s something that can be handled in a follow-up that focuses just on standardizing the code examples in the docs, ideally with a lint rule to enforce future consistency.

[IlyasShabi]

Do you think this PR is ready to be merged, or does it still need additional work?

[anonrig]

Do you think this PR is ready to be merged, or does it still need additional work?

I'm sorry I missed this. I think we can merge it.

[nodejs-github-bot]

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

[IlyasShabi]

@anonrig Could you please run the CI ?

[nodejs-github-bot]

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

[GeoffreyBooth]

What if https://github.com/nodejs/node/pull/53060 lands? And someone passes both --env-file and --env-file-optional. Does this flag affect both? Would there be any way to specify “override local variables for one .env file, but not for the other”? (Or do we need such an ability?)

[BoscoDomingo]

What if #53060 lands? And someone passes both --env-file and --env-file-optional. Does this flag affect both? Would there be any way to specify “override local variables for one .env file, but not for the other”? (Or do we need such an ability?)

I'd argue for the former: it's simpler, more consistent (if you pass multiple --env-file this flag already affects them all) and at that point users are trying to do a fairly complex operation for which a specialised tool/script may be more suitable, but open to feedback