Skip to content
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

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

Open
leolannenmaki opened this issue Sep 27, 2017 · 6 comments

Comments

@leolannenmaki
Copy link

Commit that changed the behaviour 0c5774f

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: #234 (comment)

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

@mhamann
Copy link
Collaborator

mhamann commented Sep 27, 2017

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...

@leolannenmaki
Copy link
Author

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.

@ErisDS
Copy link

ErisDS commented Oct 18, 2017

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.

@mhamann
Copy link
Collaborator

mhamann commented Oct 18, 2017

@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...

@ErisDS
Copy link

ErisDS commented Oct 18, 2017

@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.

@lobabob
Copy link

lobabob commented Jun 17, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants