(#133) Add option allowEmptyValues #134

mkrause · 2018-06-27T19:00:12Z

No description provided.

mrsteele · 2018-07-23T13:16:33Z

src/index.js

@@ -47,7 +48,11 @@ class Dotenv {

    Object.keys(blueprint).map(key => {
      const value = vars.hasOwnProperty(key) ? vars[key] : env[key]
-      if (!value && safe) {
+
+      const isMissing = typeof value === 'undefined' || value === null ||


Thanks again @mkrause for this, I think i'm hesitant to merge this in because readability is challenging.

Before we had if (!value && safe) and no we have if (safe && isMissing) which is a little less straitforward.

Maybe if we could set it up in such a way that stays closer to the ease in readability, like if (!value && required) so we can know exactly why this error would be thrown just by looking at the code.

Let me know if you have questions.

Is the issue in the isMissing assignment or in the if statement?

Because the following seems clear to me:

if (safe && isMissing) { ... }

This just says "if we're in safe mode, and the variable is considered "missing", then throw the "Missing var" error". Whether a variable is considered missing depends on the allowEmptyValues flag, if it's true then an empty string is considered missing (in addition to the other possible falsy values, null and undefined).

Would it help if I changed the isMissing declaration to an if statement instead? Something like:

let isMissing = !value if (!allowEmptyValues) { isMissing = isMissing && (value !== '') }

@mrsteele Thoughts on the above? :)

Hey @mkrause I haven't forgotten about you.

Real job has been busy so I haven't had time to circle back to this guy.

So i'm trying to come up with a simple psuedo-if statement that highlights what I think is most readable, and as I try to do that I realize I personally do not understand the use-case of allowEmptyValues. Perhaps you can help me understand it.

This is the best pseudo-if I can come up with:

// make sure value is set, or at least declared when allowEmptyValues is true const isSet = value || (allowEmptyValues && value === '') if (safe && !isSet) { // error }

So i'm trying to come up with a simple psuedo-if statement that highlights what I think is most readable, and as I try to do that I realize I personally do not understand the use-case of allowEmptyValues. Perhaps you can help me understand it.

So, this is to allow environment variables to be empty without causing a failure. An example would be a configuration value for an optional feature. Maybe I have a feature FOO which is enabled if FOO is nonempty but otherwise disabled.

Another common example is where one environment variable depends on another. For example, let's say I have a flag ANALYTICS_ENABLED to enable/disable analytics. If enabled, we might have certain other ANALYTICS_ variables to configure that feature. If analytics is disabled, you would like to leave those variables empty.

Currently these use cases require a workaround, like some placeholder value (I often use "~"). Besides being ugly, it's also dangerous as sometimes your placeholder may be a valid value.

This is the best pseudo-if I can come up with

This is essentially doing the same thing, just reversed. If you prefer this I can change it.

BTW, I can try to motivate my current approach a bit more:

I use the term "missing" instead of "set" because that's the terminology used in the error and the rest of the code.

I write out the different cases (undefined, null, empty string) rather than using value or !value because those three cases are implicit under JS's truthiness rules, so I figured since we make a distinction between empty string and the other falsy values it would be more readable to write them out explicitly.

But I'm not adamant about these choices of course. :) Would you like me to change it to your pseudocode instead?

@mrsteele Final verdict on this PR?

@mkrause You have a background in typescript/typed languages (Java)?

I'm so torn on this. On the one hand I like explicit architectures, but the "allowEmptyVariables" thing has always tripped me up.

Aside from dotenv-safe, can you find other plugins/systems that use this "explicitely-defined-empty approach"?

@mkrause You have a background in typescript/typed languages (Java)?

I've worked with both dynamically typed languages (JS, Python, etc.) and statically typed languages (C/C++, Java, JS+flow, Haskell). And I'm a fan of functional programming. :)

Aside from dotenv-safe, can you find other plugins/systems that use this "explicitely-defined-empty approach"?

I don't think you have to look much further than JS itself really. How do you handle empty values in JS? You define the variable and initialize it using null or perhaps an empty value like an empty string.

// getFoo has return type `null | string` const options = { foo: getFoo() }; if (options.foo !== null) { doSomethingWithFoo(options.foo); }

The point is that foo is always defined, it just may be empty. Compare it to Maybe/Option types in languages like Haskell, Java, Swift, etc. Or nullable types in TypeScript/Flow.

In contrast, the approach you mentioned in your previous comment to "leave it out of the .env.example" would correspond to leaving a property out of the options object. In TypeScript you'd have to define the type of options to be something like { foo ?: string }. In many other type systems you couldn't even do this without introducing a custom type for options. It's possible, but awkward.

What are your thoughts on the falsy principles in js? I'm personally a big fan, and I think it helps with readability and architecture design philosophies.

Changing a few names around:

// getFoo has return type `null | string` const options = { path: getPath() || __dirname }; if (!options.path) { throw new Error('You have no path!') }

In my above example, we have used falsy principles to be intentionally inclusive of potential problem-values. The error would be thrown if path is null, undefined, 0, false or even "". Checking explicitly for null is restrictive and potentially problematic when you forget about the other falsy values. With your developmental architecture you could force the data to flow in such a way to only allow string | null but you now have to cary the weight of remember you used null instead of any of the other falsy values.

I think it's a double-edged sword. I make use of the kinds of shortcuts you mention if it's clear what's happening. For example, if we have a variable of type null | object then using foo || defaultValue is a good idiomatic way to handle defaults.

If on the other hand it's not immediately obvious (e.g. a variable has a complex type like undefined | null | string | string[] or something), then I'll handle the different cases explicitly. Relying too much on JS's weak typing rules is an easy way to get bitten by bugs, I know from experience.

sunitJindal · 2018-12-26T12:49:45Z

@mkrause this PR needs your attention

mkrause · 2018-12-26T23:15:44Z

@sunitJindal I can fix the conflicts. But from the conversation with @mrsteele it doesn't seem like it's going to be merged. So I don't think it's worth the effort.

@mrsteele Let me know if you want to merge this, or otherwise decline it.

mrsteele · 2018-12-27T00:53:47Z

@mkrause it’s not a matter of should and shouldn’t, it’s more of the fact that I think I want these types of configurations to be a bigger thing.

There are a few major players in the dotenv ecosystem I want to include and I think this approach exposes how we are “one-offing” these configs instead of extending. I’m going to take a quick approach at what I’m thinking and get back to you...

mrsteele · 2018-12-27T02:03:18Z

Okay @mkrause I realize now what the problem is...

I think we need to make some changes to the way we currently configure the dotenv-safe portions of this package, specifically allowing the safe property to be an object that can represent the dotenv-safe configuration.

So what I am suggesting is that this PR would specifically restructure the dotenv-safe related configs to be nested into the "safe" configuration property.

By default, safe would be set to false, if true it would use the dotenv-safe defaults, if a string it would use that as a path (so we don't break anything), and we would also support an object that would then mimic the additional functionality from the dotenv-safe configs.

Here should be the updated config format for this module:

path ('./.env') - The path to your environment variables.
silent (false) - If true, all warnings will be suppressed.
systemvars (false) - Set to true if you would rather load all system variables as well (useful for CI purposes).
expand (false) - Allows your variables to be "expanded" for reusability within your .env file.
safe (false) - If false ignore safe-mode, if true load './.env.example', if a string load that file as the sample, otherwise use an object to configure additional features.
safe.example (./.env.example) - The path to the safe file.
safe.allowEmptyValues (false) - Allows empty variables to still pass the evaluation phase.

Sorry for all this delay, but after spending some time away from this I think that this is what I was mostly hung up on, but couldn't quite put my finger on it till now.

Thoughts / feedback?

mkrause · 2018-12-27T23:03:22Z

@mrsteele Interesting, thanks! The proposed config structure looks fine to me.

Unfortunately I don't think I have the time to implement this though, would probably require a bit more time than I'm willing to put into it at the moment. (Also I'm leaving for vacation tomorrow :) )

mrsteele · 2018-12-27T23:24:26Z

Alright, no big deal. Either someone else can get to it or maybe you can find some time when you get back. Either way, have a good vacation! Matt Steele On Dec 27, 2018, at 6:03 PM, mkrause <[email protected]<mailto:[email protected]>> wrote: @mrsteele<https://github.com/mrsteele> Interesting, thanks! The proposed config structure looks fine to me. Unfortunately I don't think I have the time to implement this though, would probably require a bit more time than I'm willing to put into it at the moment. (Also I'm leaving for vacation tomorrow :) ) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#134 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AFA4RNoYTJgHr9PWSU5KSw0AN6dzoovsks5u9VG6gaJpZM4U6OLB>.

vladimirlogachev · 2019-01-15T17:06:14Z

Thank you for your work!
I just want to say that this change is necessary.

mkrause · 2019-03-08T16:40:05Z

Just a quick update: for my own projects I've switched to webpack-dotenv-plugin, since this supports an allowEmptyValues config through dotenv-safe. I'm afraid I won't be continuing work on this issue, but if someone wants to pick this up please do.

…mpty-values

codecov · 2020-05-10T20:38:42Z

Codecov Report

Merging #134 into master will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #134      +/-   ##
==========================================
+ Coverage   98.57%   98.59%   +0.02%     
==========================================
  Files           1        1              
  Lines          70       71       +1     
==========================================
+ Hits           69       70       +1     
  Misses          1        1

Impacted Files	Coverage Δ
src/index.js	`98.59% <100.00%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 13a3e12...8120722. Read the comment docs.

mrsteele · 2020-05-11T14:49:20Z

🎉 This PR is included in version 1.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

mkrause added 2 commits June 27, 2018 20:55

(mrsteele#133) Add option allowEmptyValues.

04970c8

(mrsteele#133) Add README example for allowEmptyValues.

ea632a1

mkrause mentioned this pull request Jun 27, 2018

Allow empty vars in safe mode #133

Closed

Merge branch 'master' into issue-133-allow-empty-values

9f29490

mrsteele reviewed Jul 23, 2018

View reviewed changes

Merge branch 'master' into issue-133-allow-empty-values

f8781c1

mkrause added 2 commits May 10, 2020 21:50

Merge remote-tracking branch 'mrsteele/master' into issue-133-allow-e…

2e06fcf

…mpty-values

(allowEmptyValues) Update PR to be compatible with latest master.

8120722

mrsteele approved these changes May 11, 2020

View reviewed changes

mrsteele merged commit 0744700 into mrsteele:master May 11, 2020

mrsteele added the released label May 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(#133) Add option allowEmptyValues #134

(#133) Add option allowEmptyValues #134

mkrause commented Jun 27, 2018

mrsteele Jul 23, 2018

mkrause Jul 23, 2018

mkrause Aug 14, 2018

mrsteele Aug 25, 2018

mkrause Aug 28, 2018

mkrause Sep 10, 2018

mrsteele Sep 10, 2018

mkrause Sep 10, 2018

mrsteele Sep 10, 2018

mkrause Sep 10, 2018

sunitJindal commented Dec 26, 2018

mkrause commented Dec 26, 2018

mrsteele commented Dec 27, 2018

mrsteele commented Dec 27, 2018

mkrause commented Dec 27, 2018

mrsteele commented Dec 27, 2018 via email

vladimirlogachev commented Jan 15, 2019

mkrause commented Mar 8, 2019

codecov bot commented May 10, 2020

mrsteele commented May 11, 2020

(#133) Add option allowEmptyValues #134

(#133) Add option allowEmptyValues #134

Conversation

mkrause commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunitJindal commented Dec 26, 2018

mkrause commented Dec 26, 2018

mrsteele commented Dec 27, 2018

mrsteele commented Dec 27, 2018

mkrause commented Dec 27, 2018

mrsteele commented Dec 27, 2018 via email

vladimirlogachev commented Jan 15, 2019

mkrause commented Mar 8, 2019

codecov bot commented May 10, 2020

Codecov Report

mrsteele commented May 11, 2020