GeezerVison

Review requested:

• @nodejs/startup

@MoLow The feature is under Stability: 1.1 - Active development.

This requires documentation.

@targos Can you review again?

I don’t feel strongly enough about this to block it, but I want @nodejs/tsc to review so that we get a broader opinion of what we think the desired UX should be: either this PR’s warning or the current behavior’s error or the opt-in --env-file-optional of #53060. Let’s go with whichever option the most people think feels most expected. I doubt we need a formal vote, but hopefully some discussion in the next TSC meeting should reveal at least among the TSC what most people expect.

I don’t feel strongly enough about this to block it, but I want @nodejs/tsc to review so that we get a broader opinion of what we think the desired UX should be: either this PR’s warning or the current behavior’s error or the opt-in --env-file-optional of #53060. Let’s go with whichever option the most people think feels most expected. I doubt we need a formal vote, but hopefully some discussion in the next TSC meeting should reveal at least among the TSC what most people expect.

Sounds good to me 👍

I wont block but I'm -1 on this change.
If you use a flag to load a file and that file does not exist it should throw. I understand it's less flexible and apparently has slightly worse dx but it's more solid and secure and might save you in production

While I agree that errors should be preferable, I think establishing a pattern is also important.

• --openssl-config=file - Does not throw an error if the file is not found
• NODE_EXTRA_CA_CERTS=file - Does not throw an error if the file is not found. It shows a warning instead
• NODE_ICU_DATA=file - Does not throw an error if the file is not found
• SSL_CERT_FILE=file + --use-openssl-ca - Does not throw an error if the file is not found

While I agree that errors should be preferable, I think establishing a pattern is also important.

• --openssl-config=file - Does not throw an error if the file is not found

• NODE_EXTRA_CA_CERTS=file - Does not throw an error if the file is not found. It shows a warning instead

• NODE_ICU_DATA=file - Does not throw an error if the file is not found

• SSL_CERT_FILE=file + --use-openssl-ca - Does not throw an error if the file is not found

What if they all threw a warning, and a new flag, like --throw-on-file-not-found (or similar) would cause them to instead throw. Same for this flag

I'm fine with landing it, we should probably discourage production use, and explicitly mention that is a development tool and using in production might cause side effect if you forget to configure it properly

The TSC discussed this in the TSC meeting today and there were no blockers so this can land.

Doesn't it need to support well all of these use-cases?

I don't know what the main use case for --env-file is. I see that @anonrig added a thumbs-up to my previous comment, but I don't know if that means "it's only meant for local development so ignoring errors is somewhat fine."

If folks use this flag in production, then I strongly believe that ignoring user errors by default is a mistake.

Also, the behavior of this flag was changed specifically because users requested the current behavior (i.e., an error), and both polls mentioned above resulted in the majority of respondents saying that they'd expect an error.

I would prefer having that configurable (i.e. having a flag to override the default behavior).

If we’re going to have two flags, I think it’s much more useful to have a flag for each behavior. Then you can have a required .env file and an optional one, e.g.:

node --env-file=base.env --env-file-optional=local.env app.js

Doesn't it need to support well all of these use-cases?

I don't know what the main use case for --env-file is. I see that @anonrig added a thumbs-up to my previous comment, but I don't know if that means "it's only meant for local development so ignoring errors is somewhat fine."

@tniessen dotenv is mostly used on development not on production. env-file is also aimed to be used for development environment.

The polls don’t reflect the reality unless a correct sample size is achieved to reflect reality.

And what size would that be? #53177 (comment) has 27 votes, and @nodejs/collaborators has 88 members.

Hey there, author of #53060 here. I wasn't subscribed to this thread, so I hadn't seen any of this until now, sorry.

Seeing as the TSC didn't reach any consensus, and the outcome was "discuss it async in the PR", I'm adding my 2 cents.

Honestly, it seems clear to me the majority of users expect an error to be thrown - the 2 polls ran showed >60% of voters picking it. I personally agree with @GeoffreyBooth and prefer that option.

If I'm not mistaken, proposed solutions boil down to

This throwing an error if file doesn't exist (current behaviour):

node --env-file .env app.js

This showing a warning if file doesn't exist but executing anyways (this PR):

node --env-file .env app.js

Adding a new flag --env-file-optional to explicitly mark certain files as optional (#53060, my PR)

node --env-file .env --env-file-optional optional.env app.js

Adding a new flag to this PR that can toggle "strict mode" (not implemented):

node --env-file .env --strict app.js # error
node --env-file .env app.js # warning

Obviously I favour the 3rd approach, as it is the least disruptive (not that it matters at this point, feature is experimental after all) and allows for the highest freedom with the least setup (no duplication of scripts in package.json or touch .env shenanigans, etc) while also providing the same behaviour as dotenv and related packages.

Maybe we could add a warning if the file doesn't exist and was marked as optional as @jasnell proposed, appeasing that crowd too? It doesn't atm.

Also, my PR was approved by @mcollina but is blocked by @anonrig. Not sure up to what degree there is a "conflict of interest" here. I do agree Node collaborators' input should be regarded more highly than the general public, but in this case one could argue my PR is also ready to land, so I'm not sure if that's a strong argument in favour of this one in this regard; we're kind of at a stalemate.

On a side note, I'm surprised at the amount of discussion such a small change sparked and I'm happy to see such a thriving community chime in and do their best to improve such a widely used tool!

Also, my PR was approved by @mcollina but is blocked by @anonrig. Not sure up to what degree there is a “conflict of interest” here. I do agree Node collaborators’ input should be regarded more highly than the general public, but in this case one could argue my PR is also ready to land, so I’m not sure if that’s a strong argument in favour of this one in this regard; we’re kind of at a stalemate.

@anonrig would you be willing to drop your block on #53060? It seems that the majority of users would prefer the error behavior, but we have a substantial minority that want the warning behavior, so I think the reasonable thing to do would be to land #53060 to satisfy both constituencies.

@anonrig would you be willing to drop your block on #53060? It seems that the majority of users would prefer the error behavior, but we have a substantial minority that want the warning behavior, so I think the reasonable thing to do would be to land #53060 to satisfy both constituencies.

Here's my personal thoughts on this: Most people have node -r dotenv/config in their package.json as dev and run npm run dev etc. On top of that, they provide a default value for the environment variables in the case they don't exist. Throwing an error would disrupt this flow (which I and almost all companies I've worked for has been used for a really long time). This forces people to push .env files into their git flow with either default values and/or as an empty file.

@GeoffreyBooth I still don't think we should maintain 2 separate CLI flags for a development specific functionality. When we first merged --env-file most people approved the feature with the condition of keeping the implementation as simple as possible. Now, we are dropping this. --env-file= with --env-file-optional= or any other naming, seems overkill for a feature like this. Passing environment variables from a file should never be encouraged for a production application (because of filesystem security etc. etc). Supporting both environments at the same time will encourage people to use --env-file while making the maintenance and complexity a lot harder for a handful of C++ developers who are contributing to Node.js.

If you still insist, I'd like @nodejs/tsc to take a vote on this.

My personal preference is the status quo so I'm fine with neither PR landing; but I won't block. I think we should respect our user feedback. If we build this feature at all, it should do what users want.

@anonrig I totally see your point here, but anecdotal evidence is a tough sell:

Here's my personal thoughts on this: Most people have node -r dotenv/config in their package.json as dev and run npm run dev etc. On top of that, they provide a default value for the environment variables in the case they don't exist. Throwing an error would disrupt this flow (which I and almost all companies I've worked for has been used for a really long time).

For example I've never seen such a setup, rather most code I've seen made use of dotenv.config() at runtime to attempt to load a .env file if it did in fact exist. Since we're not removing dotenv, but rather adding a built-in alternative, supporting the existing functionality via the --env-file-optional version while also providing a stricter one is an overall improvement over the existing version.

This forces people to push .env files into their git flow with either default values and/or as an empty file.

No need - they can just use the optional flag and keep everything else as is: node --env-file-optional .env main.js instead of node -r dotenv/config.
(Also note the current situation does force touch .env or duplicating scripts as stated in #51451, so it is the worst of both worlds and something that should be addressed).

@GeoffreyBooth I still don't think we should maintain 2 separate CLI flags for a development specific functionality. When we first merged --env-file most people approved the feature with the condition of keeping the implementation as simple as possible. Now, we are dropping this. --env-file= with --env-file-optional= or any other naming, seems overkill for a feature like this. Passing environment variables from a file should never be encouraged for a production application (because of filesystem security etc. etc). Supporting both environments at the same time will encourage people to use --env-file while making the maintenance and complexity a lot harder for a handful of C++ developers who are contributing to Node.js.

These are 2 very fair points I'll concede to. My implementation is objectively more complex, and having a -optional would make it seem like the regular flag is "for prod". However, the complexity is not such that it makes you go "wtf is this code" (I was able to build it with almost 0 prior knowledge of C++) nor is the Node team responsible for how people choose to use the tools it provides, IMO.

If you still insist, I'd like @nodejs/tsc to take a vote on this.

I agree, but as @GeoffreyBooth said, I'd like to also somehow take users' desires into account. We're never going to get 100% of Node users to vote on such a small issue (or any for that matter), but we should take action somehow.

We either

• let the TSC dictate the direction for this,
• we listen to user feedback (no matter how small the sample size, plus it likely comes from the most dedicated Node users who actually chime into topics like this one) or
• a combination of both (open to suggestions)

most code I’ve seen made use of dotenv.config() at runtime to attempt to load a .env file if it did in fact exist.

For what it’s worth, this is possible already:

import { readFile } from 'node:fs/promises'
import { parseEnv } from 'node:util'

try {
  const envFileContents = await readFile('.env', 'utf8')
  parseEnv(envFileContents)
} catch {}

The one thing this doesn’t do is affect Node itself if there’s a NODE_OPTIONS environment variable defined in the .env file. But that could be handled in various ways, such as introducing config file support with the ability to define different configurations by condition. (Or in today’s Node, by starting a child process with --env-file when fs.existsSync tells us that the file exists.)

Seeing as my PR has received two -1s at this stage, I'm personally ok with merging this one in favour of at least improving the current situation, even if it goes against the polls. We can always revisit later with more feedback

I'm personally ok with merging this one in favour of at least improving the current situation, even if it goes against the polls. We can always revisit later with more feedback

FWIW, we have revisited the feature, which originally only produced a warning. Then, based on more feedback, it was changed to the current behavior. Recent polls, albeit with a small sample size, also indicated that this was preferred by (some subset of) the community. So I am not convinced that merging this PR would actually improve on the status quo.

I'm personally ok with merging this one in favour of at least improving the current situation, even if it goes against the polls. We can always revisit later with more feedback

FWIW, we have revisited the feature, which originally only produced a warning. Then, based on more feedback, it was changed to the current behavior. Recent polls, albeit with a small sample size, also indicated that this was preferred by (some subset of) the community. So I am not convinced that merging this PR would actually improve on the status quo.

@tniessen Fair point. Hoewver, it does force users to either have duplicated scripts (one with, the other without --env-file) or to create the file beforehand if it is not present, which is hacky. In any case, I do understand if that is outside of the scope of what Node should care about. That being said, I wouldn't be surprised if new issues were opened every now and then regarding an optionality toggle of some sort. If that's fine with you guys, we can simply leave it as-is?

@tniessen Why can't we support both options? Throwing a non-negligible subset of the community under the bus would be a fairly questionable approach. This doesn't seem to be the kind of a feature that benefits from being opinionated, especially having in mind that, as @anonrig has pointed out, .env is very frequently used in non-prod environments.

@tniessen Why can't we support both options?

@kibertoad I don't think I've said anything against supporting both options. (Or perhaps I did a long time ago and don't remember.)

I'd like to add a new dimension to the discussion from a frontend/full-stack perspective. While I agree with @anonrig's argument that .env files are not used in production environments, this is generally prevalent in the backend world of Node.js.

As you might know, on the frontend side, popular tools like Vite and Next.js process environment files with slightly different dynamics. In such codebases, helper scripts or code pieces may need to leverage the --env-file capabilities that could be added to Node.js.

References:

• https://vitejs.dev/guide/env-and-mode#env-files
• https://nextjs.org/docs/pages/building-your-application/configuring/environment-variables#environment-variable-load-order

Therefore, in a use-case where small scripts are utilized to trigger Prisma/ORM migrations (especially the rise of server-side rendering made full-stack codebases quite popular), we might need to use commands like node --env-file=.env --env-file=.env.local --env-file=.env.production --env-file=.env.production.local ./etc/script.js. (of course, not all of these files will necessarily be present)

I may have expanded the scope a lot... but I thought it should be considered when making a decision.

Closing given the recent TSC vote: https://github.com/nodejs/TSC/blob/main/votes/2024-06-25-0.json