-
Notifications
You must be signed in to change notification settings - Fork 44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add store.bind(), remove $env directive, misc fixes #110
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -224,7 +241,7 @@ internals.coerce = function (value, type, options) { | |||
break; | |||
case 'array': | |||
if (typeof value === 'string') { | |||
result = value.split(options.splitToken); | |||
result = value ? value.split(options.splitToken) : []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch on that one, completely forgot this edge case, when splitting an empty string it'll return an array of a single empty string....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only noticed it because this case previously wasn't hit by the test suite. When I removed the process.env-patching code in the test suite, this popped up with a test failure :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice feature @devinivy . I like it.
Closing this for now, as I don't see it going anywhere soon. The bug in this PR has since been addressed 👍 |
Consider this a proposal to address #106: I understand that this work may not be used depending on whether we decide to keep
$env
or not.The proposal here is to remove
$env
, while introducing a way to bind criteria to the store. I consider this a generalization of$env
because we can still use$param
to reference environment variables. The only thing missing was a way to make certain criteria "intrinsic" to the store (i.e. doesn't need to be explicitly passed tostore.get()
) in the same way thatprocess.env
was. I personally like this approach because it's both more generic than$env
and more explicit. It also means that our confidence stores are more testable because bindings can be overridden, whereas before you would have to workaround this by patchingprocess.env
. I found that the confidence test suite even had an issue due to the code it uses to patchprocess.env
, and removing this code uncovered a bug!Here's an example usage for a user who wants environment variables bound to their store:
Additional changes in here:
''
now results in[]
rather than['']
(this was theprocess.env
-related bug mentioned above). The test suite actually expected''
to be interpreted as missing with a fallback to default (though that was never the actual behavior), but this means there was no way to represent an empty array. So I sort of split the difference.__proto__
are considered invalid.store.load()
(andstore.bind()
) return the store for fluent-style chaining.