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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: let ConfigService type know about schema validation #676

Merged
merged 3 commits into from Nov 3, 2021
Merged

feat: let ConfigService type know about schema validation #676

merged 3 commits into from Nov 3, 2021

Conversation

micalevisk
Copy link
Member

@micalevisk micalevisk commented Sep 23, 2021

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

[ ] Bugfix
[x] 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?

Issue Number: #668

What is the new behavior?

Adds a second optional type arg to ConfigService generics.

Users now can tell to ConfigService that some validation was made on ConfigModule#forRoot, so there's no room for ConfigService#get to return undefined when we narrow it to some type like:

// ...
// after making sure that you set up `ConfigModule#forRoot` with `validationSchema`
// ...

interface EnvironmentVariables {
  PORT: number;
  TIMEOUT: string;
}

// ...
constructor(private configService: ConfigService<EnvironmentVariables, true>) {
  //                                                                   ^^^^ Telling to ConfigService
  //                                                                        that everything was validated
  const port = this.configService.get('PORT');
  //    ^^^ the type of this cannot be `undefined` anymore, just `number`
}

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other information

I could update the https://docs.nestjs.com/techniques/configuration after merging this PR

* If some typings doesn't follows the requirements, Jest will fail due to
* TypeScript errors.
*/
private noop(): void {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this strategy is that bad tho. Since I don't want to add libs such as tsd or dtslint to check if my proposing feature for ConfigService is working as expected (and will keep working in the future), I've added this method to do the type checking as a side-effect of running npm run test:integration :p

I could drop this commit if you want to. Or I could add one more to test the this.configServiceNarrowed.get(x) use case (ie., relying on type inference)

Since there's no other way to check TypeScript types within this project
without addind some 3rd-party lib, I've made this approach: let TSC
complains about typings while running the npm-script `test:integration`.
@kamilmysliwiec
Copy link
Member

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants