Order of reading configuration values favours environment variables over custom configuration

[sorooshme]

Is there an existing issue for this?

• I have searched the existing issues

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

sorooshme#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?

• macOS
• Windows
• Linux

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]

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

[sorooshme]

I agree.

Glad to hear that.

let's wait till the next major release.

How are major versions coordinated / released? couldn't find about it in the contribution doc.

If no major release is planned yet, would you be okay with adding this change in a non-breaking way? we can put it behind a flag that's disabled by default (so we keep the current behaviour) and on the next major version, we clean it up.

I'll be happy to raise a PR for it, wdyt @kamilmysliwiec ?

[kamilmysliwiec]

#1883