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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(): do not remove undefined from ConfigService.get return type #1304

Merged

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?

The return type of a validated ConfigService.get excludes undefined. This hides null errors in the following case.

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

const opt = configService.get('OPT', {infer: true}) // type is: string, should be string | undefined
const hasCat = opt.includes('馃惐') // Will result in runtime error because opt is undefined

Issue Number: #1302

What is the new behavior?

The return type now no longer excludes undefined if WasValidated is true.

Does this PR introduce a breaking change?

  • Yes
  • No

Users that have strictNullChecks enabled, have optional properties on their Env class, and depend on the behavior of ConfigService.get stripping undefined from the return type will receive type errors. Example of code that would now result in errors:

class Env {
    mandatoryString?: string
}

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

The TypeScript error raised:

src/file.ts:??:20 - error TS2532: Object is possibly 'undefined'.

??     const hasCat = configService.get('mandatoryString', {infer: true}).includes('馃惐')
                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

I would argue that their types are wrong and this is exposed by this PR.

The return type now no longer excludes undefined if `WasValidated` is
true. Previously, optional properties were stripped of `undefined`
which hid possible null errors.

BREAKING CHANGE: Users that have strictNullChecks enabled, have
  optional properties on their Env class, and depend on the behavior
  of ConfigService.get stripping undefined from the return type will
  receive type errors.
@micalevisk
Copy link
Member

just to clarify here, can you share what's the result of this:

interface Env {
  optionalString?: string
  mandatoryString: string
  nilField: null | undefined
}

const configService = app.get<ConfigService<Env, true>>(ConfigService)

const opt = configService.get('optionalString', {infer: true}) // type ??
const man = configService.get('mandatoryString', {infer: true}) // type ??
const nil = configService.get('nilField', {infer: true}) // type ??

@MatthiasKunnen
Copy link
Contributor Author

The types from your example are:

const opt = configService.get('optionalString', {infer: true}) // type string | undefined
const man = configService.get('mandatoryString', {infer: true}) // type string
const nil = configService.get('nilField', {infer: true}) // type null | undefined

@kamilmysliwiec kamilmysliwiec merged commit c574d93 into nestjs:master May 16, 2023
1 check passed
@kamilmysliwiec
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants