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

.env file support issue tracker #49148

Open
7 of 8 tasks
anonrig opened this issue Aug 13, 2023 · 56 comments
Open
7 of 8 tasks

.env file support issue tracker #49148

anonrig opened this issue Aug 13, 2023 · 56 comments
Labels
discuss Issues opened for discussions and feedbacks. dotenv Issues and PRs related to .env file parsing

Comments

@anonrig
Copy link
Member

anonrig commented Aug 13, 2023

This is a follow-up of dotenv support pr to track the development process, and answer some questions that require a general discussion for making the decision to implement them.

Todos:

Questions:

  • Should we merge NODE_OPTIONS if both environment variable and .env version exist? Currently, we prefer environment variable over .env configuration. (cc @ljharb) src: merge env-file and env vars of NODE_OPTIONS #49217
  • Should we throw if .env file does not exist? We currently follow the default behavior of dotenv package and do not throw. (cc @MoLow)

Please feel free to edit this issue in case there are more questions that needs to be answered.

cc @nodejs/tsc

@anonrig anonrig added the discuss Issues opened for discussions and feedbacks. label Aug 13, 2023
@benjamingr
Copy link
Member

I vote "throw on non-existing env file" and "throw on multiple --env-file parameters" since those are easy to change later and lets us land without waiting for a decision on both.

@gireeshpunathil
Copy link
Member

IMO:

Should we merge NODE_OPTIONS if both environment variable and .env version exist?

The usual practice (of environment variable precedence ordering) is command line overriding stored setting in files. That way, the end user who runs the code has the last say on the settings.

Should we support multiple --env-file through the CLI?

Yes. This will help co-existence of application and its dependencies with their own env definitions.

Should we add a programmatic API?

Yes. This will help embedded use cases where node.js is not started through the regular launcher with regular command line parsing sequences.

Should we throw if .env file does not exist?

Yes. This will help the feature function to be deterministic.

@tniessen
Copy link
Member

Should we add a programmatic API?

Yes. This will help embedded use cases where node.js is not started through the regular launcher with regular command line parsing sequences.

I assume that this refers to a C++ API for embedders. This might be useful if the API can be used before any of Node.js or any of its dependencies are initialized. Otherwise, embedders will run into the same problem as we did with NODE_OPTIONS.

@gireeshpunathil
Copy link
Member

I assume that this refers to a C++ API for embedders.

Yes, I meant that.

This might be useful if the API can be used before any of Node.js or any of its dependencies are initialized. Otherwise, embedders will run into the same problem as we did with NODE_OPTIONS.

can you please elaborate on the problem we did with NODE_OPTIONS? sorry, I am not up-to-date with that.

@tniessen
Copy link
Member

What I mean is that environment variables that affect Node.js, its dependenices, the embedding application, or the embedder's dependencies must be set before the respective component attempts to use it. In many cases, that means before initialization of the respective component.

(On the other hand, reading environment variables from a file is simple enough, so if an application doesn't use the Node.js CLI, it probably might as well implement this feature itself.)

@GeoffreyBooth
Copy link
Member

What I mean is that environment variables that affect Node.js, its dependenices, the embedding application, or the embedder’s dependencies must be set before the respective component attempts to use it. In many cases, that means before initialization of the respective component.

Yes. As in, it doesn’t do much good to write JavaScript code like process.env.NODE_OPTIONS = '...' because by the time this JavaScript code runs, Node has already loaded and configured itself and so most (all?) of what you would set by this point is too late to have any effect.

Re merging versus overwriting, I feel somewhat strongly that we should overwrite, because this .env file support is general purpose for any environment variable and not just Node-specific ones, and we can’t know how to merge variables like DATABASE_PASSWORD or whatever. I think it would be confusing UX if Node environment variables got merged but non-Node ones didn’t. I was assuming that whatever variables were set in the environment would take precedence over ones set in a loaded file, but if others feel strongly that it should be the reverse I don’t have a strong opinion on this part.

@ljharb
Copy link
Member

ljharb commented Aug 14, 2023

I would assume that individual variables would overwrite but that if the env has A and the file has B that the process would end up with both A and B.

@anonrig
Copy link
Member Author

anonrig commented Aug 14, 2023

I would assume that individual variables would overwrite but that if the env has A and the file has B that the process would end up with both A and B.

@ljharb This is the default behavior right now, but do not merge NODE_OPTIONS.

➜  node git:(dotenv-support) cat .env
TESTING_KEY=THIS_IS_VALUE
➜  node git:(dotenv-support) TESTING_KEY=OVERRIDEN ./out/Release/node -e "console.log(process.env.TESTING_KEY)"
OVERRIDEN

@ljharb
Copy link
Member

ljharb commented Aug 14, 2023

ah ok, gotcha. then yeah i think #49148 (comment) makes sense because indeed it would get confusing figuring out which node options are mutually exclusive, which are single values, which are multiple values, etc.

@RomainLanz
Copy link
Contributor

I was wondering, do you plan to support also something like dotenv-vault that lets you encrypt your secret and decrypt it just in time.

@anonrig
Copy link
Member Author

anonrig commented Aug 16, 2023

I was wondering, do you plan to support also something like dotenv-vault that lets you encrypt your secret and decrypt it just in time.

Not at the moment, but contributions are welcome.

@philnash
Copy link
Contributor

philnash commented Aug 31, 2023

I was just seeing how this feature worked and I think that either the documentation or the feature is wrong in the 20.6.0 release.

The docs say:

If the same variable is defined in the environment and
in the file, the value from the environment takes precedence.

But, I checked out the v20.6.0-proposal branch, built the latest executable and tried the following:

ϟ echo $PHIL_VAR
what
node (4e4bb9f) 
ϟ ./node --version
v20.6.0
node (4e4bb9f) 
ϟ ./node -e "console.log(process.env.PHIL_VAR)"                
what
node (4e4bb9f) 
ϟ ./node -e "console.log(process.env.PHIL_VAR)" --env-file .env
hello
node (4e4bb9f) 
ϟ cat .env
PHIL_VAR=hello

From the documentation, I would have expected the PHIL_VAR variable set in the environment to always be the result. Instead, it seems that the .env file version of the PHIL_VAR variable over-rides it when it is used.

Does this need a quick documentation fix ("the value from the .env file takes precedence") or does the feature need fixing?

@GeoffreyBooth
Copy link
Member

does the feature need fixing?

The feature needs fixing. The environment should take precedence over the file.

@mcollina
Copy link
Member

mcollina commented Sep 4, 2023

The environment should take precedence over the file.

I don't think so. The whole point of env files is to override the local environment.

@philnash
Copy link
Contributor

philnash commented Sep 4, 2023

The environment should take precedence over the file.

I don't think so. The whole point of env files is to override the local environment.

We've discussed this quite a bit in this pull request, but every existing implementation of dotenv, including dotenv-node and bun, does not override the environment by default. They tend to have an option to override, which we should consider implementing, but to make overriding the default would go against every existing implementation.

@mcollina
Copy link
Member

mcollina commented Sep 4, 2023

My bad!

@kibertoad
Copy link
Contributor

@anonrig Big and important use-case for programmatic API is setup hooks for testing frameworks, such as vitest or jest.

@anonrig
Copy link
Member Author

anonrig commented Sep 7, 2023

I opened a PR to address multiple --env-file declarations support: #49542

@anonrig Big and important use-case for programmatic API is setup hooks for testing frameworks, such as vitest or jest.

Thanks @kibertoad. It is on my agenda.

@tniessen
Copy link
Member

tniessen commented Sep 8, 2023

Big and important use-case for programmatic API is setup hooks for testing frameworks, such as vitest or jest.

@kibertoad Previously, the programmatic API was described as a C++ API for embedders. Is that what you have in mind? If not, could you clarify?

@kibertoad
Copy link
Contributor

@tniessen I mean being able to put an equivalent of require('dotenv').config() inside a hook :D

@tniessen
Copy link
Member

tniessen commented Sep 8, 2023

In that case, this discussion has already requested two very different runtime APIs. The JS runtime API likely does not need to live in core since it won't be able to properly set NODE_OPTIONS etc.

@kibertoad
Copy link
Contributor

@tniessen That would be an understandable limitation of JS API. But if native .env support can't be used from JS userspace, then users will still need to depend on dotenv for tests, and then there are no good reasons not to use dotenv for everything else as well, for consistency.

nodejs-github-bot pushed a commit that referenced this issue Sep 10, 2023
PR-URL: #49542
Refs: #49148
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Chemi Atlow <[email protected]>
@mbrevda
Copy link

mbrevda commented Jan 12, 2024

Should we add a programmatic API? If yes, what would be the use-case? (cc @cjihrig)

One use case could be to load a conditionally .env only if it exists, a feature that was removed in #50588. See also #50993

@anonrig
Copy link
Member Author

anonrig commented Jan 26, 2024

Is there any feature/behavior change request before making the dotenv functionality stable?

@GeoffreyBooth
Copy link
Member

Is there any feature/behavior change request before making the dotenv functionality stable?

We should maybe do the “define the .env file format” one, as otherwise the supported format is “whatever --env-file supports” which isn’t really a spec, and then any improvements would be breaking changes (like if we add support for multiline values). If you can find some external definition to point to, great, but if not then I guess we should define our own definition based on how the code parses the file, and in our definition we can mention potential future improvements like multiline files, substitutions, encryption, etc.

We should also decide if we want .env loaded automatically. That would be a semver-major change, but we could land such a PR now to ship with Node 22 in April. I’m not sure if we do want that, since .env is a very common filename and so this could be a very surprising breaking change for a lot of users. If we finish the work to support a Node config file, we could have a filename like .config.node.json loaded automatically in Node 22, and that JSON file could define all the Node options including an envFile option.

@IlyasShabi
Copy link
Contributor

Another feature we might support is the dotenv package's override option Do you know how often this is used ?

@GeoffreyBooth
Copy link
Member

Another feature we might support is the dotenv package’s override option

Adding that shouldn’t be a breaking change, so it could happen after marking the API as stable.

@nikeee
Copy link
Contributor

nikeee commented Feb 6, 2024

We should maybe do the “define the .env file format” one, as otherwise the supported format is “whatever --env-file supports”

docker compose has something spec-ish for env files:
https://docs.docker.com/compose/environment-variables/env-file/

Lines breaks have to be done using \n + quotes. Maybe it is possible to align with the same spec, so env-files could be re-used etc.

@sosoba
Copy link
Contributor

sosoba commented Feb 22, 2024

Is there a reason why only relative paths are supported? For example, when I want to load a secret in a container I am forced to count dots:
node --env-file=../../run/secrets/envs
instead of:
node --env-file=/run/secrets/envs

@targos
Copy link
Member

targos commented Feb 22, 2024

@sosoba support for absolute paths was added in #51425. It will be in the next v21 release.

@kibertoad
Copy link
Contributor

kibertoad commented Mar 28, 2024

@IlyasShabi override option is a prerequisite for having incremental config files (e. g. .env + .env.test with test-specific overrides), which is a pretty common pattern.
You can use download stats for https://www.npmjs.com/package/dotenv-flow to see that it is quite popular.

@OpportunityLiu
Copy link

@jaydenseric
Copy link
Contributor

I've read the Node.js --env-file docs, and searched the Node.js GitHub issues and PRs, and am struggling to find out if Node.js supports environment variable interpolation/expansion, like:

https://www.npmjs.com/package/dotenv-expand#what-rules-does-the-expansion-engine-follow

A lot of projects use that, so having an upfront answer if it is supported or not, or if it ever might be supported, would be useful for people migrating projects from dotenv-expand to --env-file. If Node.js plans to support interpolation in the future, we will just wait for it before dropping dotenv-expand. If not, we will think seriously about avoiding interpolation in our .env files and adopt --env-file now.

@anonrig
Copy link
Member Author

anonrig commented Apr 30, 2024

@jaydenseric I'm not opposed to adding it. For making env file stable, we don't need it, but at any time pull-requests are welcome.

@acidoxee
Copy link

I don't know whether it's a bug or if it's just that comments are not really a supported feature yet, but comments in .env files have a weird behavior. Here are some results I've gotten with Node v20.12.2 for the command node --env-file .env -e "console.log(process.env.FOO)":

.env content Expected Result OK?
undefined undefined
FOO=1 1 1
# FOO=1 undefined 1
FOO=1
# FOO=2
1 1
# FOO=2
FOO=1
1 1
FOO=1
# FOO=2
# FOO=3
1 3
# FOO=2
FOO=1
# FOO=3
1 1
# FOO=2
# FOO=3
FOO=1
1 1
FOO=1
# FOO=2
# FOO=3
# FOO=4
1 3
# FOO=2
FOO=1
# FOO=3
# FOO=4
1 4
# FOO=2
# FOO=3
FOO=1
# FOO=4
1 1
# FOO=2
# FOO=3
# FOO=4
FOO=1
1 1
FOO=1
# FOO=2
# FOO=3
# FOO=4
# FOO=5
1 5
# FOO=2
FOO=1
# FOO=3
# FOO=4
# FOO=5
1 4
# FOO=2
# FOO=3
FOO=1
# FOO=4
# FOO=5
1 5
# FOO=2
# FOO=3
# FOO=4
FOO=1
# FOO=5
1 1
# FOO=2
# FOO=3
# FOO=4
# FOO=5
FOO=1
1 1

The simple # FOO=1 doesn't seem to work, and for more complex cases, it seems the proper value is only provided for last and before-last positions, otherwise it's wrong (and the wrong yielded value also depends on the position of the non-commented declaration).

It's quite surprising and potentially dangerous, since someone might keep commented out values like this in order to be able to switch between them easily:

DB_URL=<dev URL>
# DB_URL=<staging URL>
# DB_URL=<prod URL>

As I've shown in the table, the value of DB_URL here won't be <dev URL>, it will be <prod URL>.

@IlyasShabi
Copy link
Contributor

@acidoxee, we recently addressed a PR to fix issues with parsing comments. Could you please check with Node v22?

@targos
Copy link
Member

targos commented Apr 30, 2024

@IlyasShabi I guess you're talking about #52406. It's not part of any release yet.

@IlyasShabi
Copy link
Contributor

@targos yes my bad sorry

@BoscoDomingo
Copy link
Contributor

Should we throw if .env file does not exist? We currently follow the default behavior of dotenv package and do not throw. (cc @MoLow)

Yes (and that's already implemented in #50588) but I'd argue in favour of an --optional-env-file argument too. Can be extremely useful in cases where we know there might not be one and we're ok with it, such as production environments where the container already has the environment variables set up and no .env file is needed. Related Feature Request: #50993.

I'm working on it, but I haven't touched C++ since University (and even then only on a very basic level) so any help with this topic will be appreciated. Currently checking all related PRs to try and build on that 👍🏼

@anonrig
Copy link
Member Author

anonrig commented May 19, 2024

@BoscoDomingo I'll open a PR to revert the error. I agree that we shouldn't throw an error if env file is not found as a cli argument.

@BoscoDomingo
Copy link
Contributor

BoscoDomingo commented May 19, 2024

@BoscoDomingo I'll open a PR to revert the error. I agree that we shouldn't throw an error if env file is not found as a cli argument.

@anonrig I'm on it right now, so maybe wait a bit and review mine if you want? Currently writing tests and it should be ready. Also, I do believe the --env-file should error. What I'm proposing a completely new arg (that maybe throws a warning instead?)

Edit: here you go #53060

@RedYetiDev
Copy link
Member

FWIW #53461 has some ambiguities in the parsing that might need clarification / adjustment.

@RedYetiDev RedYetiDev added the dotenv Issues and PRs related to .env file parsing label Jun 15, 2024
@RedYetiDev
Copy link
Member

I'm sure I'm missing something here, but is there a reason Node.js doesn't use a dependency for Dotenv?

@GeoffreyBooth
Copy link
Member

I'm sure I'm missing something here, but is there a reason Node.js doesn't use a dependency for Dotenv?

Part of the goal was to support NODE_OPTIONS as a de facto config file, which can only happen if the parsing is done by core before V8 starts.

@macrozone
Copy link

it does not seem to support the same .env files like dotenv: #54134

since the format is so problematic, maybe a different fileformat would be better? maybe just supporting json as this is already a familiar format and is already built-in in node?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discuss Issues opened for discussions and feedbacks. dotenv Issues and PRs related to .env file parsing
Projects
None yet
Development

No branches or pull requests