src: don't overwrite environment from .env file #49424
Conversation
This commit adds a check to see if an environment variable that is found in the .env file is already set in the environment. If it is, then the value from the .env file is not used.
nodejs-github-bot
added
c++
|
This seems to contradict what the average expectation would be when using the On a side note, can you link to where you found that quote? I couldn't find that anywhere in the linked thread. The closest I saw was an incomplete action item that said needed more discussion. |
I agree with @TacoZilla here. I think we should change the docs, not the code. More locally defined config should take precedence over less locally defined config. |
|
All the existing implementations of dotenv behave this way. You can see that the Node.js dotenv library has an I think this feature should follow the prior implementations. Though we should also consider a way to override the environment for a future release too. |
|
I agree with @philnash. I didn't had any time to check the code but I think this is the correct path. |
|
TIL. I agree we should follow what all of the other implementations do. Side note: It seems like a design decision with obvious downsides. I'm curious if there are advantages I'm just missing to overriding by default other than the consistency aspect. |
|
The significant difference to me is that libraries like dotenv are used/required/imported during runtime in application code. This feature is a CLI option you pass to the node executable. While I understand their end goals are similar (e.g. expose environment variables to your application), I'd argue the implementation matters. If node was offering a module that you used similarly to dotenv, I'd be fine matching the existing behavior, but it feels wrong in the current implementation. This conflicts (albeit maybe not in the conventional way of thinking) with the precedent that CLI options always override environment variables:
On one hand, you could make the argument this isn't actually violating that because the option is always used even if the end result is a no-op, but on the other hand, I'd argue it does violate things in spirit. If I pass a CLI option, I expect it to have highest priority / override anything previously set, but it's entirely possible I'm just weird :P There should definitely be some way to override existing environment variables, so I think everyone at least agrees on that in some form. |
|
Honestly, I'm sort of torn here. I've never really liked the default way that any dotenv implementation works, I expect my local settings to override the environment. I've been caught out by that many times before and I don't know why that was the original decision that was taken. However, as I've said above, it is the default for many, if not all, implementations of dotenv. If Node is to provide an implementation of dotenv, even a simple one compared to the options that are out there, then I think it should match what already exists. I think that if someone who is using dotenv-node with default settings decides to drop it in place of using And that's why I wrote this pull request. It goes against how I think the feature should work, but it follows how everyone who does know the feature already knows how it works. (And yes, we should add override as an option somehow, but that can be done in a further PR.) |
anonrig
added
author ready
github-actions
bot
removed
the
request-ci
|
Important note: I think we need to make an exception for $ NODE_OPTIONS='--env-file=./.env' node something.jsI think we should override the |
NODE_OPTIONS has already an exception (defined at 769823e#diff-6c1386cbc6e4a11941649af31a0257d5c3e40b5f41223cdddf87ab2e984b705aR851), but you're right that we need to also make an exception in here, since we want to get the correct value for |
GeoffreyBooth
added
the
cli
I disagree, I think Yes we need to prevent an infinite loop, but that should be the only exceptional handling that we do.
My reading of this is that |
I understand the sentiment. I think you can achieve your desired behavior, at least in Bash and similar shells, via: set -a; source .env; set +a; node app.jsWhich is a bit clunky but gets the job done. Maybe this is why the default is the way it is in those other libraries. |
Isn't that argument somewhat self-defeating? Using the same argument, you could ask why node is implementing this as an option in the first place? With that being said, I think framing this as should node match dotenv is missing the point. A better question is why dotenv implementations behave the way they do and then ask if that same logic holds true for this case. From the Ruby implementation's docs:
I would argue that reasoning doesn't hold true for node's implementation. Passing a CLI option to the node executable would live pretty firmly in the context of the deployment environment, as such, it should take precedence. At the end of the day, if node's priority is simply to blindly match dotenv as closely as possible, then this entire discussion is moot. I don't agree with blindly matching for the sake of matching, but I do understand that line of reasoning, but in that case I think we should be very clear and acknowledge it's not necessarily the actual best way to implement it. On a slight tangent, I really don't understand when this default behavior would be useful in the average case. If a user is explicitly passing in an environment file, the chances of then wanting already set environment variables to take precedence would be very low if not directly contradict using the CLI option to begin with. |
In the initial discussions for the feature we had discussed making loading That doesn’t mean we couldn’t have one precedence behavior for autoloaded files versus CLI-referenced files, but I’m assuming that we wouldn’t want such a divergence in behavior.
Because there’s a difference between
What the Ruby docs describe is exactly how I’d use this feature myself. I can imagine having a ROOT_URL=http://localhost:3000
DATABASE_USER=postgres
DATABASE_PASS=postgresAnd these obviously wouldn’t be the values in the prod environment. I could have an |
|
One more point of comparison: Bun behaves like echo 'FOO=bar' > .env
echo 'console.log(process.env.FOO)' > test.js
bun test.js # bar
FOO=baz bun test.js # baz |
philnash
force-pushed
the
src-dont-overwrite-environment-from-dotenv
branch
from
ae5cb39 to
271bebd
Compare
|
Another fail. This one is a problem In this case the environment set the |
|
@nodejs/platform-aix @nodejs/platform-smartos Any idea why this is failing? |
In some CI environments the TZ env var is set. Since a .env file won't override that, we need to create an env where the test can still run by deleting the TZ variable.
philnash
force-pushed
the
src-dont-overwrite-environment-from-dotenv
branch
from
4565fa2 to
56f0868
Compare
github-actions
bot
removed
the
request-ci
|
I think I'm getting the hang of analysing the Jenkins builds. This fail seems to be the flaky IPv6 test again. |
|
Live view of me hoping this time the CI will pass. I think one of the suites just straight failed to run this time. |
|
|
atlowChemi
added
the
commit-queue
nodejs-github-bot
removed
the
commit-queue
|
Landed in 015e27b |
This commit adds a check to see if an environment variable that is found in the .env file is already set in the environment. If it is, then the value from the .env file is not used. PR-URL: #49424 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
This commit adds a check to see if an environment variable that is found in the .env file is already set in the environment. If it is, then the value from the .env file is not used. PR-URL: #49424 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
This commit adds a check to see if an environment variable that is found in the .env file is already set in the environment. If it is, then the value from the .env file is not used. PR-URL: nodejs#49424 Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com> Reviewed-By: Geoffrey Booth <webadmin@geoffreybooth.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Chemi Atlow <chemi@atlow.co.il>
This commit adds a check to see if an environment variable that is found in the .env file is already set in the environment. If it is, then the value from the .env file is not used.
The docs from #48890 say:
In my testing, I found that not to be the case, so I went to look for a solution.
Caveats. I am not a C++ programmer and have never tried to read, understand or contribute back to a C++ code base before. This is a naive solution. Please feel free to criticize and correct my C++.
I know the v20.6.0 release is very close, but as dotenv support is a notable feature, I think this should join the release so that we don't push a feature that contradicts its own documentation.