Skip to content

Conversation

@MatthiasKunnen
Copy link
Contributor

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

  • Environment variables are assigned back to process.env resulting in unexpected stringification. This stringification is deprecated and might result in a runtime error in the future. See https://nodejs.org/api/process.html#processenv.
  • The return value for an undefined variable from ConfigService.get is the string 'undefined'.

Issue Number: Closes #1302, address part 1.

What is the new behavior?

  • Only strings are assigned back to process.env
  • Undefined variables result in undefined as return type from ConfigService.get

Does this PR introduce a breaking change?

  • Yes
  • No, unless someone was depending on [Object object] in process.env

After validation, environment variables are assigned back to
process.env. This leads to stringification of the values. This
stringification is deprecated and might result in a runtime error in
the future. See https://nodejs.org/api/process.html#processenv.

A special case is when an environment variable has the value
`undefined`. This causes `ConfigService.get` to fall back on
process.env thereby returning the stringified form `'undefined'`.
Assigning a value to process.env[] leads to stringification of the
value. Example:

```
> process.env.test = {}
> process.env.test
'[object Object]'
```

This problem was hidden because `ConfigService.get` returns the value
of the validated config which would not be stringified.

A special case is when an environment variable has the value
`undefined`. This is because `ConfigService.get` falls back to
process.env which returns the stringified form `'undefined'`.

The stringification is deprecated and might result in a runtime error
in the future. See https://nodejs.org/api/process.html#processenv.

The argument has been changed to Record<string, unknown> to be more
type-safe. If this type was used in the original code, this bug could
have been avoided.
@kamilmysliwiec kamilmysliwiec merged commit 7973ee2 into nestjs:master Jun 12, 2023
@kamilmysliwiec
Copy link
Member

LGTM

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

Successfully merging this pull request may close these issues.

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

2 participants