dotenv usage question

[ibratoev]

dotenv usage question

[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!