dotenv usage question

[ibratoev]

Current State

Currently dotenv.config is called inside the cleanEnv method like that:

const env = extend(dotenv.config({ silent: true }), inputEnv)

First, let's check the dotenv.config 2.0 documentation: "Config will read your .env file, parse the contents, and assign it to process.env."

So, what happens is that if you call cleanEnv({ foo: 'bar' }, spec) and there is a .env file available this will change your process.env state. I find this kind of unexpected.

Using this library for a while, I feel that the goal of cleanEnv is to clean and validate the environment that is passed as the first parameter with the spec provided as the second parameter. I don't really see how dotenv.config fits.

How to fix this? Here are my ideas for the next major version.

Proposal

The current cleanEnv method can be renamed to cleanProcessEnv and the first parameter can be removed. Generally, it can have this simple implementation:

dotenv.config(); // This will load the .env file and update `process.env`.
return cleanEnv(process.env, spec); // create a clean env (dotenv.config is called in cleanEnv)

cleanEnv should not call internally dotenv.config().

As an added benefit, this will be compatible with the latest dotenv version.

What do you guys think?

[af]

This is a really good point, I don't like that dotenv changes process.env, and I agree it'd be better if we didn't do that.

I think we can take an even simpler approach though, and maintain the existing cleanEnv() API (I like passing in process.env as an argument, as it's more explicit and makes for easy testing). The latest dotenv version provides a parse() function that returns an object of env vars, rather than mutating process.env. Inside cleanEnv() we could do something like this:

const cleanEnv = (inputEnv, config) => {
    const dotEnvVars = require('dotenv').parse(dotenvBuffer)
    const fullEnv = Object.assign({}, inputEnv, dotEnvVars)
    // Rest of validation logic here
}

Loading the .env file will add a bit of complexity but I think it's worth it to avoid mutating global state and keeping the API small and tidy. Thoughts?

[ibratoev]

That's another way to do it, although I think that the whole envalid library would be much more reusable if it is just doing cleanup and validation. For example, what you propose wouldn't be able to handle my case:

I would like to have variables defined on the following levels ordered by highest priority:

• environment variables (process.env);
• .env file variables;
• default deploy environment variables (variables defined in code per deployment environment - live, test,etc);
• envalid spec defaults;

If cleanEnv calls dotenv internally it doesn't allow me to compose my environment configuration. If you insist on keeping the current behavior, I can propose adding another method or an option that disables the dotenv usage.

A few more notes about your example:

• Technically, the removal of the dotenv.config call is a breaking change ;)
• The example above has a bug because dotEnvVars would overwrite inputEnv and that's not how dotenv works - environment variables are with higher priority than variables from the .env file. You can check how it is implemented in dotenv.

[af]

Sorry about the bugs, I was just sketching something quickly without actually looking at the existing code :) Yes, my example is a breaking change. I think it's worth it though to prevent dumping more stuff on process.env, and further enforcing the discipline of using a validated env object instead of accessing process.env directly.

You bring up some good points about making the environment more composable. I'm definitely not against adding a new API that gives this kind of control, I think it's a good idea. I think I'd like to continue calling dotenv inside cleanEnv() though, since that's what the users of this library are already used to. I welcome suggestions on this new API!

[ibratoev]

Maybe just add a new option to cleanEnv - loadDotenv ? So to call cleanEnv without loading dotenv you can do it like that:

envalid.cleanEnv(process.env, spec, { loadDotenv: false });

Sounds good?

[af]

@ibratoev Yep that sounds good 👍

[af]

@ibratoev Pushed some new commits to master for this (the next release will be 3.0 as this is a breaking change). Note that I changed the new option to dotEnvPath for some added flexibility; you can set it to null and dotenv processing will be skipped. I think these changes resolve this issue, let me know if otherwise.

[ibratoev]

👍
@af, even better that you can change the .env path now ;)
I will send you a PR with the Typescript declarations later this week ;)

[af]

Sounds good, thanks!