Using optional environment variables leads to 'undefined' string value and incorrectly inferred type

[MatthiasKunnen]

Is there an existing issue for this?

• I have searched the existing issues

Current behavior

One of my environment variables, OPT, is optional. I use the ConfigModule.validate function which returns an object with the OPT key having undefined as value. The following problems occur:

Using ConfigService.get('OPT') returns the string value 'undefined'

This is because ConfigModule.forRoot assigns all validated config key-value pairs to process.env which leads to stringification and consequent ConfigService.get returning the stringified value from process.env

The stringification is caused here:

config/lib/config.module.ts

Lines 194 to 203 in d368ed1

	private static assignVariablesToProcess(config: Record<string, any>) {
	if (!isObject(config)) {
	return;
	}
	const keys = Object.keys(config).filter(key => !(key in process.env));
	keys.forEach(
	key => (process.env[key] = (config as Record<string, any>)[key]),
	);
	}

Return type from ConfigService<EnvironmentVariables, true>.get excludes undefined

The second generic parameter (WasValidated) changes the return value of ConfigService.get. If true, it removes undefined from the return type. If false, it adds undefined. The problem here is that a value could be both validated and undefined. Observe the TypeScript return type of optValue in main.ts.

Minimum reproduction code

https://github.com/MatthiasKunnen/nest-config-service-optional-env-var

Steps to reproduce

No response

Expected behavior

1

assignVariablesToProcess should not assign 'undefined' or any object value to process.env. The current behavior could even result in runtime errors in the future as stringification of non-string/number/boolean is deprecated. See docs:

Assigning a property on process.env will implicitly convert the value to a string. This behavior is deprecated. Future versions of Node.js may throw an error when the value is not a string, number, or boolean.
— https://nodejs.org/api/process.html#processenv

I'm actually quite intrigued as to why the validated config is assigned to process.env? It could contain complex values such as instances of classes which could not lend themselves to being stringified.

2

The return type of ConfigService.get should include undefined for optional properties of validated configs.

Package version

2.3.1

NestJS version

9.0.11

Node.js version

18.16.0

In which operating systems have you tested?

• macOS
• Windows
• Linux

Other

Optional environment variables are not a strange concept. They should work without hiccups and could benefit from documentation at https://docs.nestjs.com/techniques/configuration.

[MatthiasKunnen]

IMO, it would make more sense if the return type of an unvalidated ConfigService.get were to be unknown.

Example

class Env {

    @IsInt()
    PORT!: number;
}

const configService: ConfigService<Env> = app.get(ConfigService);
const port = configService.get('PORT', {infer: true})

The type of port is number | undefined but since validation has not occurred it is actually string | undefined. Another example is when using more complex config parsing where a string is turned into an instance of a class on validate. Both seem to be fine type wise but could easily result in runtime errors.

In general, the ConfigService could be improved with a more strict-by-default approach, though this will be a breaking change.

[micalevisk]

but since validation has not occurred it is actually string | undefined

but that's because you're using env. vars, right? I could have a config with hard coded values as well. Then, the source of true must be that Env schema.

That's why the default is works like that and probably won't change. Is a way to avoid boilerplate code for common use cases.

There are others libs for such task in nestjs ecosystem that could satisfy your needs, see https://github.com/nestjs/awesome-nestjs

[MatthiasKunnen]

@micalevisk, I don't really understand what you're trying to say.

Then, the source of true must be that Env schema.

The fact is, the return type is incorrect and what I'm guessing you're saying is that this is on purpose because there would be more burden on users of Nest that do not care about type safety?

If this is the case, it seems troubling to me. That being said, the main problems addressed in this issue still stand regardless.

Going back to your example, say that your hardcoded config has an optional property, the validated return type would be wrong.

Example:

class Env {
    OPT?: string
}
const configService = app.get<ConfigService<Env, true>>(ConfigService);

const opt = configService.get('OPT', {infer: true}) // Is: string, Should be string | undefined

I will have a look at https://github.com/Nikaple/nest-typed-config from the awesome-nestjs repository you linked.

[micalevisk]

in your last example, we indeed got a wrong type. I don't recall exactly why we remove the undefined from Env['OPT'] type when wasValidated is true without taking into account the optional marker, tho... Might be a limitation or due to the fact that we must cover the following as well:

interface Env {
  optionalString?: string
  mandatoryString: string
}

const configService = app.get<ConfigService<Env, true>>(ConfigService);
const opt = configService.get('optionalString', {infer: true}) // must be string | undefined
const mand = configService.get('mandatoryString', {infer: true}) // must be string

[MatthiasKunnen]

The removal undefined could be fixed by changing the following:

config/lib/config.service.ts

Lines 16 to 19 in f1abd2d

	type ExcludeUndefinedIf<
	ExcludeUndefined extends boolean,
	T,
	> = ExcludeUndefined extends true ? Exclude<T, undefined> : T | undefined;

to

type ExcludeUndefinedIf< 
   ExcludeUndefined extends boolean, 
   T, 
 > = ExcludeUndefined extends true ? T : T | undefined; 

This works for the example you provided and for mine.

Regarding breaking changes, this would result in type errors for users with strictNullChecks: true and the following code:

class Env {
    mandatoryString?: string
}

const configService = app.get<ConfigService<Env, true>>(ConfigService);
const hasCat = configService.get('mandatoryString', {infer: true}).includes('🐱')

However, I would argue that their types are wrong and this was hidden by the Exclude<T, undefined>. Depending on NestJs' policy this could be seen as a breaking change.

[micalevisk]

I'm fine with such breaking change for the next major release. Let's see what Kamil thinks on this. You could open a PR already if you want to.

Thanks!

[MatthiasKunnen]

@micalevisk, I'm definitely willing to create a PR incorporating this change. One thing to note though is that while this change fixes the 2nd point of the issue, it does not fix point 1:

Using ConfigService.get('OPT') returns the string value 'undefined'

Do you want me to hold off from creating a PR until that point is addressed/discussed/triaged or would you like a separate PR that only addresses the types (point 2)?

I already have an idea about how to fix point 1 but I'm not sure why the config is assigned back to process.env (see assignVariablesToProcess) and as such do not know if there is a use case that I'm missing and would unknowingly influence.

[micalevisk]

I'm not entirely sure on why is this either. So you can create a PR for point 2 only, for now

[kamilmysliwiec]

I'm actually quite intrigued as to why the validated config is assigned to process.env? It could contain complex values such as instances of classes which could not lend themselves to being stringified.

assignVariablesToProcess is called only for variables loaded from the .env file, which should never contain any complex values. The reason why they're assigned to the process.env is that this function mimics the behavior of the dotenv package (originally it was calling dotenv.config() instead of reading the file manually).

As for 2 let's track this here #1304

[MatthiasKunnen]

I believe this is closed prematurely as one of the issues is not addressed.

assignVariablesToProcess should not assign 'undefined' or any object value to process.env. The current behavior could even result in runtime errors in the future as stringification of non-string/number/boolean is deprecated. See docs:

Assigning a property on process.env will implicitly convert the value to a string. This behavior is deprecated. Future versions of Node.js may throw an error when the value is not a string, number, or boolean.
— https://nodejs.org/api/process.html#processenv

You say that [env files] should never contain any complex values. What would be wrong with an env file like this:

SOME_JWT=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJzdWIiOiIxMjM0NTY3ODkwIiwibmFtZSI6IkpvaG4gRG9lIiwiaWF0IjoxNTE2MjM5MDIyfQ.SflKxwRJSMeKKF2QT4fwpMeJf36POk6yJV_adQssw5c

where upon parsing, the SOME_JWT value is converted/deserialized to an instance of class JwtToken.

Even if you were to put this aside, having an optional environment variable leads to ConfigService.get('OPT') returning the string value 'undefined'.

I would be willing to make a PR to help clear this up and fix this issue.

[kamilmysliwiec]

where upon parsing, the SOME_JWT value is converted/deserialized to an instance of class JwtToken.

For this, you should rather use a configuration factory/namespace, but I see where you're coming from so I would be more than happy to see a PR addressing this use-case