(#133) Add option allowEmptyValues #134
Conversation
src/index.js
Outdated
| @@ -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 || | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 !== '')
}
There was a problem hiding this comment.
@mrsteele Thoughts on the above? :)
There was a problem hiding this comment.
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
}
There was a problem hiding this comment.
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
valueor!valuebecause 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?
There was a problem hiding this comment.
@mrsteele Final verdict on this PR?
There was a problem hiding this comment.
@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"?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@mkrause this PR needs your attention |
|
@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. |
|
@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... |
|
Okay @mkrause I realize now what the problem is... I think we need to make some changes to the way we currently configure the So what I am suggesting is that this PR would specifically restructure the By default, Here should be the updated config format for this module:
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? |
|
@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 :) ) |
|
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>.
|
|
Thank you for your work! |
|
Just a quick update: for my own projects I've switched to webpack-dotenv-plugin, since this supports an |
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
🎉 This PR is included in version 1.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
No description provided.