Skip to content
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

Merged
merged 4 commits into from
Jun 17, 2024
Merged

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jun 14, 2024

@github-actions github-actions bot changed the base branch from main to initiateNewVote June 14, 2024 10:24
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@aduh95
Copy link
Contributor Author

aduh95 commented Jun 14, 2024

/cc @anonrig @BoscoDomingo as the authors of the blocked PRs, in case you want to review the proposed options

Comment on lines +31 to +32
- 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.
Copy link
Member

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.

Copy link
Contributor Author

@aduh95 aduh95 Jun 14, 2024

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

Copy link
Member

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.

Copy link
Contributor Author

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.

votes/initiateNewVote/_EDIT_ME.yml Outdated Show resolved Hide resolved
votes/initiateNewVote/_EDIT_ME.yml Outdated Show resolved Hide resolved
votes/initiateNewVote/_EDIT_ME.yml Outdated Show resolved Hide resolved
@benjamingr
Copy link
Member

benjamingr commented Jun 14, 2024

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 dotenv with this behavior,
python (and when they removed the warning at theskumar/python-dotenv#57 ) and go.

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).

@anonrig
Copy link
Member

anonrig commented Jun 14, 2024

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%.

@BoscoDomingo
Copy link

BoscoDomingo commented Jun 14, 2024

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

@benjamingr
Copy link
Member

Basically my stance isn’t warn, ignore or error - it’s “do whatever userland does”.

Copy link
Member

@benjamingr benjamingr left a 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

Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@GeoffreyBooth
Copy link
Member

Basically my stance isn’t warn, ignore or error - it’s “do whatever userland does”.

According to what you’ve written, though, dotenv has a default behavior that can be overridden by its config file. We don’t have config file support, though, so at the moment we can’t really mimic dotenv‘s behavior, right?

I guess that could be another option, though: add support for a general Node config file, and then --env-file has the warn behavior by default and it can be set to either throw or be silent via the config file. That would exactly match dotenv, I think?

@benjamingr
Copy link
Member

@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

@GeoffreyBooth
Copy link
Member

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 --env-file-optional or --env-file-required because by the time you can run process.loadEnvFile or util.parseEnv, Node is already running and therefore any NODE_OPTIONS within the .env file has no effect. Part of the existing use case for --env-file is as a way to define Node configuration via NODE_OPTIONS.

@benjamingr
Copy link
Member

That’s not equivalent to --env-file-optional or --env-file-required because by the time you can run process.loadEnvFile or util.parseEnv, Node is already running and therefore any NODE_OPTIONS within the .env file has no effect.

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.

@aduh95 aduh95 merged commit de78d34 into initiateNewVote Jun 17, 2024
3 checks passed
@aduh95 aduh95 deleted the open-env-file-vote branch June 17, 2024 15:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.