nestjs / config

Configuration module for Nest framework (node.js) 🍓
https://nestjs.com
MIT License
520 stars 91 forks source link

Order of reading configuration values favours environment variables over custom configuration #1822

Open sorooshme opened 4 weeks ago

sorooshme commented 4 weeks ago

Is there an existing issue for this?

Current behavior

Imagine the following custom configuration:

process.env.CUSTOM_FEATURE_ENABLED = 'true'; // environment variables are always a string - loaded by k8s for example

export default () => ({
  "CUSTOM_FEATURE_ENABLED": process.env.CUSTOM_FEATURE_ENABLED === 'true'
})

Loading this into the ConfigModule and then reading the value using:

const value = configService.get('CUSTOM_FEATURE_ENABLED');`

leads to the value being string true instead of a boolean value, this happens because the ConfigService tries to load the value from the environment variables first (instead of preferring to load from the custom configuration)

Minimum reproduction code

https://github.com/sorooshme/config/pull/1

Steps to reproduce

The PR includes tests that confirm this (unexpected) behaviour.

Expected behavior

I expect the environment variables to be checked last, not first.

Let me know if you think otherwise.

Package version

3.2.3

NestJS version

No response

Node.js version

No response

In which operating systems have you tested?

Other

Not using the same name as the config property name and the environment variable name could solve this, cause there won't be a clash.

But still, I believe this is a major issue, we should let the developer choose whatever property name they want, without the knowledge of their runtime environment variables.

I imagine the PR for this will be fairly straight forward (just changing the order of read), so I can submit it once we agree that this needs attention.

kamilmysliwiec commented 1 day ago

I agree. Since this would be a breaking change, let's wait till the next major release.