nconf icon indicating copy to clipboard operation
nconf copied to clipboard

conf.get('UPPERCASE') with lowerCase option used to work in 0.8.4 but not in 0.8.5

Open leolannenmaki opened this issue 8 years ago • 6 comments

Commit that changed the behaviour https://github.com/indexzero/nconf/commit/0c5774fec798e50c5749e8de6f6f77875fef2b02

Previously you could do both conf.get('NODE_ENV') and conf.get('node_env') when the lowercase option is enabled but nowadays the code in the above commit doesn't add lowercased versions of the variables to process.node.env in addition to whatever case the variables are there but instead makes a new empty object and populates that with only the lowercased versions.

The previous behaviour has been discussed at least here: https://github.com/indexzero/nconf/issues/234#issuecomment-274269340

Could we revert to the previous behavior where lowerCase option just adds the lowercased versions?

leolannenmaki avatar Sep 27 '17 09:09 leolannenmaki

I'm thinking that perhaps env lookups should be totally case-insensitive (if lowerCase === true). Potentially, that should even apply across every store, as it's probably a bug (or at least a code smell) if your config has two keys with the same name in two different cases...

mhamann avatar Sep 27 '17 19:09 mhamann

Yeah, total case-insensitivity might make sense. I don't really want to define bothNoDe_EnV and node_env :) Or maybe there could be an ignoreCase or caseInsensitive option. Not sure.

One app broke because we were setting lowerCase to true and on the next line conf.get('NODE_ENV'). So we kinda did unknowingly depend on the previous behavior and were pretty surprised when things got wonky after the upgrade.

leolannenmaki avatar Sep 28 '17 08:09 leolannenmaki

I'd like to also add my vote in favour of making it possible to have proper case insensitivity. We had someone submit a PR to Ghost, which adds the lowerCase: true option.

The problem with this, in our opinion, is that so many of our keys are camelCased that we'd end up having duplicates of most keys & values stored in the config object. This is not just untidy but would impact on memory usage unnecessarily.

The real underlying problem, at least for us, is that people want to be able to supply SERVER__PORT=xxxx instead of server__port=xxxx as environment variables, because lowercase environment variables feel weird.

I'm not sure what a solution to this could look like, but some ideas:

  • add support for an option such that all lookups are case insensitive, without needing to duplicate keys (treat duplicates where the only difference is case as code smell as @mhamann said)
  • support some sort of transform function, so that the input from env/argv/etc could be lowercased and also converted from snake cake to camel case.
  • maybe add an option for snake -> camel conversion for the various inputs (env, argv etc)

I also think it would be perfectly legitimate to change nconf to have 100% case insensitive lookups and call that nconf 1.0.

ErisDS avatar Oct 18 '17 17:10 ErisDS

@ErisDS we are working on the PRs and features for v0.9 and v1.0 now. No ETA, but hoping to ship it "soon."

I've been thinking about this a bit overall as well, and tend to agree that pure case insensitivity would be a good option (perhaps even the default in the next major version).

Greatly appreciate the feedback. Stay tuned...

mhamann avatar Oct 18 '17 18:10 mhamann

@mhamann Thanks for the quick reply. The Ghost Team ❤️ nconf. We have gained an enormous amount of benefit by switching to use it in Ghost 1.0 instead of our old, homemade solution. Nconf works wonderfully for us, the desire to have upper case support for env vars comes from our users rather than from us 😉 I'm really glad that there are active maintainers again though. Thank you for taking it on.

ErisDS avatar Oct 18 '17 18:10 ErisDS

@mhamann Did this ever land? The documentation seems to imply the old behavior of the lowerCase option still holds.

lobabob avatar Jun 17 '19 19:06 lobabob