Should we remove $env?

[devinivy]

I noticed that $env can be modeled equally well using $param if the user passes process.env in their criteria. I propose that in v6 we remove $env and encourage users to explicitly pass the environment as criteria if that is their intention. I am in favor of this change because it simplifies the API, and encourages the store to not rely on any implicit/global state.

I just made a small alteration to allow $coerce to work with $param to generate this example:

Before (with $env)

const store = new Confidence.Store({
    server: {
        host: 'localhost',
        port: {
            $env: 'PORT',
            $coerce: 'number',
            $default: 3000
        },
        debug: {
            $filter: { $env: 'NODE_ENV' },
            $default: {
                log: ['error'],
                request: ['error']
            },
            production: {
                request: ['implementation']
            }
        }
    }
});

const config = store.get('/');

After (with $param)

const store = new Confidence.Store({
    server: {
        host: 'localhost',
        port: {
            $param: 'PORT',
            $coerce: 'number',
            $default: 3000
        },
        debug: {
            $filter: 'NODE_ENV',
            $default: {
                log: ['error'],
                request: ['error']
            },
            production: {
                request: ['implementation']
            }
        }
    }
});

const config = store.get('/', process.env);

[Nargonath]

I use $env a lot so as long as we don't lose the feature I'm all up for it. Especially if it simplifies the codebase. It doesn't seem to overcomplicate the API. Only downside I see is that $env was more explicit to where it gets its values from compared to $param if you're not too familiar with confidence API.

[YoannMa]

We use $env as well because it's more explicit, plus we use criteria for other things like sentry/tracing agent that need to be initialized before any other require() but are needed in a plugin for biding with server's events, requests, etc

It would not be a deal breaker thought, we could just do :

const store = new Confidence.Store({
    server: {
        host: 'localhost',
        port: {
            $param: 'env.PORT',
            $coerce: 'number',
            $default: 3000
        },
        debug: {
            $filter: 'env.NODE_ENV',
            $default: {
                log: ['error'],
                request: ['error']
            },
            production: {
                request: ['implementation']
            }
        }
    }
});

const config = store.get('/', { env : process.env, other: 'things' });

Bonus point for still being explicit enough too

[Nargonath]

@YoannMa Good idea I like it that way too, plus as you mentioned it's still explicit.

[devinivy]

If we decide we want to go forward with this, I have a proposal here: https://github.com/hapipal/confidence/pull/110

I thought that removing $env left a small gap, so in that proposal I introduced a way to bind criteria to the store, store.bind(). I think it has some nice upsides, but check out the proposal if you're interested and of course feel free to offer any feedback, concerns, questions. @augnin if you have the time, I am especially interested to hear what you're thinking on all of this.

[devinivy]

Worth noting that there was some chatter in hapi hour about this, mostly concerns that removing $env would make things less explicit than it currently is, and that some folks like having this feature available to them. I would love to at least keep its feature set in parity with $param, and maintain it as just a special case of $param.

[Nargonath]

I agree. If we were to modify this feature, leveraging $param as @YoannMa mentioned above seems to be the best choice IMO.