Open devinivy opened 3 years ago
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.
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
@YoannMa Good idea I like it that way too, plus as you mentioned it's still explicit.
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.
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
.
I agree. If we were to modify this feature, leveraging $param
as @YoannMa mentioned above seems to be the best choice IMO.
I noticed that
$env
can be modeled equally well using$param
if the user passesprocess.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
)After (with
$param
)