-
Notifications
You must be signed in to change notification settings - Fork 134
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
Propose vote to unblock --env-file
PRs
#1577
Conversation
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
/cc @anonrig @BoscoDomingo as the authors of the blocked PRs, in case you want to review the proposed options |
- Throw when passed to `--env-file` flag, unless new `--env-file-when-missing=warn` is also passed. | ||
- Warn when passed to `--env-file` flag, unless new `--env-file-when-missing=throw` is also passed. |
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 don't think anyone has been advocating for these. These options would mean you can't have a mix of required and optional files to load.
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 don't think anyone has been advocating for these.
I have (see nodejs/node#53060 (comment))
These options would mean you can't have a mix of required and optional files to load.
That's correct
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.
Sorry I missed that. I feel like a bunch of people would be upset if this is what we land, because the use case they want to support is “I have some env files that every environment needs, plus another env file that only localhost needs”. That was what prompted --env-file-optional
if I remember correctly.
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 feel like a bunch of people would be upset if this is what we land
Yes probably, and that applies to all the candidates BTW – that's why we're having a vote, because we couldn't find a solution that would please everybody.
Co-authored-by: Tobias Nießen <tniessen@tnie.de>
Note none of the candidates are the behavior userland libraries have and we're effectively "dictating by committee" an API that's different from the one the ecosystem gravitated towards which is "ignore missing env file, warn if debug:true is passed". For example here is I am happy to provide more examples (this is the behavior in every package I looked at). I really dislike the project landing a feature it's adopting because it reached consensus in userland and then changing its behavior by committee even though all userland libraries (at least popular ones I found) have the same behavior (which is different from what we're proposing). |
Note: I agree with @benjamingr 100%. |
I agree as well, we just each approached the problem from a different angle. In the end we both want missing .env files to not throw, I just made it a decision to be taken by the user, whereas you prefer sticking to other tools' existing behaviour, and that's 100% fine too 👍🏼 Also, the proposal LGTM (save a small change in the title), thanks @aduh95 |
Basically my stance isn’t warn, ignore or error - it’s “do whatever userland does”. |
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 still don’t really like the options but it’s always fine to call for a vote and I appreciate your effort in making progress and unblocking here
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
According to what you’ve written, though, I guess that could be another option, though: add support for a general Node config file, and then |
@GeoffreyBooth the equivalent would be "do not warn on missing .env file, add an option to process.loadEnvFile to warn/throw on a missing env file |
That’s not equivalent to |
Correct, I was only explaining what userland does and was suggesting we keep the same existing behavior that reached consensus in our ecosystem over many years rather than change it based on how a technical committee believes things should work. |
Refs: #1575 (comment)