-
Notifications
You must be signed in to change notification settings - Fork 28.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
src: add --env-file-optional
flag
#53060
base: main
Are you sure you want to change the base?
src: add --env-file-optional
flag
#53060
Conversation
Fixes: nodejs#50993 Refs: nodejs#51451
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we shouldn't introduce a new flag. We should make the default --env-file not throw and revert the change.
@anonrig Are you sure? I am personally fine with #50588, and at this stage, we might be breaking people's CI processes and whatnot by allowing processes to run that will be missing environment variables. Multiple people in #49148 agree with this stance, too. I'd like to understand your reasoning |
This is an "active development" non stable feature. therefore, breaking changes are expected. IMHO, Instead of throwing an error, we can print a warning. I don't see why we should create a new flag rather than changing the current one. |
@anonrig I do see value in having a "hard" and a "soft" version, to allow users to decide what behaviour they desire. Plus the underlying implementation will be the same and thus barely any complexity is added while greatly improving UX/DX. In my case, there are multiple instances where I'd want the node process to be aborted if the .env file is not present. Is there any team we should tag to get their input on this as well? |
There is no specific team responsible for I think #50588 was a mistake. @mcollina and several others have expressed their opinion on this as well, but it never hurts to ask for their opinion one more time :-) FYI, I didn't block this pull-request, any collaborator can come in and approve your pull-request, and land this pull-request. |
Agree on asking others. I'm not an expert on this matter either, so happy to hear other POVs. I do err on the side of keeping |
If we add this, the flag name needs to start with I feel pretty strongly that the current behavior is correct. We should not be changing |
There seems to be a consensus that:
Is there any precedent for loader-style arguments? Perhaps prefixing (or suffixing) the path with a symbol (say, E.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@GeoffreyBooth Fair enough. You can see in #51451 the different workarounds we currently need to do. I believe for this to compete with dotenv, bun or other alternatives out there, the UX has to be at least matching, if not better. They all currently march on when the .env file is not found. We can do better. In terms of changing the flag, good on my side 👍🏼 @mbrevda I feel like that's too much complexity both in use (I'd never seen such syntax) as in implementation (what happens with |
Updated code according to the failed CI jobs. Should all be good now @GeoffreyBooth Updated flag to start with |
--optional-env-file
flag--env-file-optional
flag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 on this change. It makes the existing implementation harder to maintain for almost no gain. We should not throw on env-file instead.
I will block on changing I'm also fine with no solution landing for this problem. I'm not convinced this is something that Node should solve, as opposed to users better managing their commands for production versus development. |
We can always convert it into a warning. |
I agree with @GeoffreyBooth on this one. I'd much rather Node doesn't allow the program to run than to start it with the wrong assumptions. In my experience, startup warnings are rarely read in production environments, and usually not even properly marked as such by log services. It's far worse to let systems crash in prod due to missing files than to not start them to begin with. However, the pain for users having to manage multiple scripts for whether they want to load x file or not will be a major pain (imagine all the possible combinations) when it's an easy fix for us. Therein lies the value IMHO. Edit: @anonrig I like the idea of a warning for missing optional files |
I would opt to have the flag not fail when the file is missing, have an option to say that the file is optional, or simply detect that a |
Hello there. Basically, both of the solutions are going to achieve the same result. But I would also prefer to have the flag to not fail. I think every people using
Personnaly, I never used dot env files outside of local developments. With this solution, for example, I cannot run the tests with the same I love to minimize libraries on my microservices, I really hope it will stop to throw. BTW thank you for the contribution and all my thanks to the team! |
Just for reference: this issue will be discussed in the next TSC meeting since a consensus has not been reached. More info in the counterpart PR by @anonrig Thank you all for the input, and we'll see how this one goes! |
I assume the intended usage of this PR is: node --env-file=maybe-exists.env --env-file-optional app.js What if instead it’s: node --env-file-optional=maybe-exists.env app.js And if both |
@GeoffreyBooth No, the idea is to have
The flag was |
I would be in favor of either non-boolean flag (e.g. |
@aduh95 I personally am not a big fan of flags that affect others, plus it doesn't really solve any of the issues why I proposed this change to begin with. One would still need multiple scripts for files that may or may not exist, and need to be aware of the context in which each is run to decide if the file will exist. |
I guess I misunderstood the way the flag worked. Maybe |
@aduh95 Sounds good to me |
case Dotenv::ParseResult::FileError: | ||
errors->push_back(file_path + ": not found"); | ||
if (!file_data.is_required) continue; | ||
errors->push_back(file_data.path + ": not found"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is a pre-existing bug, this really should not print not found
if the file could not be opened due to an error other than ENOENT
. (Unfortunately, ParsePath()
just returns a generic FileError
value regardless of the error code.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. How'd you suggest we handle this, simply not print anything? Improving the check as to why it errored in the first place? I do believe it should still error if the flag used was --env-file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's fine to leave it as it is for this PR since it's a pre-existing bug. Eventually, the code should check if the error code was ENOENT
, and if it's not that, then print the appropriate error message for the error code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 too.
Add a new flag,
--env-file-optional
that allows loading .env files without throwing an error if they don't exist.Achieved by returning an
env_file_data
struct instead of just the paths of the .env files. This tells the loader if the file itself is required or not, thus allowing to not throw an error if the file doesn't exist and is optional, without breaking existing behaviours with--env-file
(we could maybe show a warning if an optional file is missing?).Usage:
If
B.env
exists, it will load its environment variables and overwrite those that conflict withA.env
. If it doesn't, node will continue as normal.Fixes: #50993
Refs: #51451