-
Notifications
You must be signed in to change notification settings - Fork 29.8k
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
Comments
I vote "throw on non-existing env file" and "throw on multiple |
IMO:
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.
Yes. This will help co-existence of application and its dependencies with their own env definitions.
Yes. This will help embedded use cases where node.js is not started through the regular launcher with regular command line parsing sequences.
Yes. This will help the feature function to be deterministic. |
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 |
Yes, I meant that.
can you please elaborate on the problem we did with NODE_OPTIONS? sorry, I am not up-to-date with that. |
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.) |
Yes. As in, it doesn’t do much good to write JavaScript code like Re merging versus overwriting, I feel somewhat strongly that we should overwrite, because this |
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 |
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. |
I was wondering, do you plan to support also something like |
Not at the moment, but contributions are welcome. |
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.
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 Does this need a quick documentation fix ("the value from the .env file takes precedence") or does the feature need fixing? |
The feature needs fixing. 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. |
My bad! |
@anonrig Big and important use-case for programmatic API is setup hooks for testing frameworks, such as vitest or jest. |
I opened a PR to address multiple
Thanks @kibertoad. It is on my agenda. |
@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? |
@tniessen I mean being able to put an equivalent of |
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 |
@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 |
PR-URL: #49542 Refs: #49148 Reviewed-By: Geoffrey Booth <[email protected]> Reviewed-By: Chemi Atlow <[email protected]>
Is there any feature/behavior change request before making the dotenv functionality stable? |
We should maybe do the “define the We should also decide if we want |
Another feature we might support is the dotenv package's override option Do you know how often this is used ? |
Adding that shouldn’t be a breaking change, so it could happen after marking the API as stable. |
docker compose has something spec-ish for env files: 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. |
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: |
@IlyasShabi |
Specs of systemd env files: https://www.freedesktop.org/software/systemd/man/latest/systemd.exec.html#EnvironmentFile= |
I've read the Node.js 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 |
@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. |
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
The simple 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 |
@acidoxee, we recently addressed a PR to fix issues with parsing comments. Could you please check with Node v22? |
@IlyasShabi I guess you're talking about #52406. It's not part of any release yet. |
@targos yes my bad sorry |
Yes (and that's already implemented in #50588) but I'd argue in favour of an 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 👍🏼 |
@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 Edit: here you go #53060 |
FWIW #53461 has some ambiguities in the parsing that might need clarification / adjustment. |
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 |
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? |
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:
.env
files, and consider a better format not based on ini--env-file
through the CLI? (cc @GeoffreyBooth) src: support multiple--env-file
declarations #49542UV_THREADPOOL_SIZE
tests on.env
#49213process.loadEnvFile
andutil.parseEnv
#51476Questions:
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 ofNODE_OPTIONS
#49217.env
file does not exist? We currently follow the default behavior ofdotenv
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
The text was updated successfully, but these errors were encountered: