hapipal / confidence

Dynamic, declarative configurations
Other
263 stars 43 forks source link

Add store.bind(), remove $env directive, misc fixes #110

Closed devinivy closed 3 years ago

devinivy commented 3 years ago

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 to store.get()) in the same way that process.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 patching process.env. I found that the confidence test suite even had an issue due to the code it uses to patch process.env, and removing this code uncovered a bug!

Here's an example usage for a user who wants environment variables bound to their store:

const store = new Confidence.Store({
    mysql: {
        host: { $param: 'env.MYSQL_HOST' },
        port: { $param: 'env.MYSQL_PORT', $coerce: 'number', $default: 3306 },
        user: { $param: 'env.MYSQL_USER' },
        password: { $param: 'env.MYSQL_PASSWORD' },
        database: { $param: 'env.MYSQL_DATABASE' }
    }
}).bind({ env: process.env });

Additional changes in here:

devinivy commented 3 years ago

Closing this for now, as I don't see it going anywhere soon. The bug in this PR has since been addressed 👍