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

Pdt24 25 | Create a wrapper for environment variables #9

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

pratham-outside
Copy link
Collaborator

Tasks

Changes

  • Created a new file to validate environment variables.
  • Use of Zod.
  • Validation on bootstrap of application
  • Changed APP_ENV to NODE_ENV
  • Edited .env.example

sonar-project.properties Outdated Show resolved Hide resolved
src/config/env.config.ts Show resolved Hide resolved
src/config/env.config.ts Show resolved Hide resolved
@noxiousghost noxiousghost changed the base branch from main to PDT24-11 December 24, 2024 06:53
@noxiousghost noxiousghost changed the base branch from PDT24-11 to main December 24, 2024 06:53
src/main.ts Outdated
await app.listen(process.env.APP_PORT ?? 3000, () => {
console.info('Listening to server....');
console.info(`Server listening at port http://localhost:${process.env.APP_PORT}`);
const port = envVariables.APP_PORT ?? 3000;
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for default value if we're already defining it in envVariables

Comment on lines 1 to 5
import { Module } from '@nestjs/common';
import { AppController } from './app.controller';
import { AppService } from './app.service';
import { validate } from './config/env.config';
import { ConfigModule } from '@nestjs/config';
Copy link

Choose a reason for hiding this comment

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

Please prefer import aliases "@/*" rather than relative imports.

It will help a lot for code organization.

})
.required();

export const validate = (): object => {
Copy link

@rrojan rrojan Dec 27, 2024

Choose a reason for hiding this comment

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

Use return type z.infer<typeof validationSchema> instead of object

@@ -0,0 +1,48 @@
import { z } from 'zod';
export const envVariables = {
Copy link

Choose a reason for hiding this comment

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

Suggested change
export const envVariables = {
export const env = {

username: process.env.POSTGRES_USER,
password: process.env.POSTGRES_PASSWORD,
database: process.env.POSTGRES_DB,
port: +(envVariables.POSTGRES_PORT ?? '5432'),
Copy link

Choose a reason for hiding this comment

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

After we return z.infer<typeof validationSchema> env.POSTGRES_PORT will not be falsey.

Comment on lines 11 to 12
database: envVariables.POSTGRES_DB,
synchronize: false, // Should be false in production to use migrations
Copy link

Choose a reason for hiding this comment

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

Suggested change
database: envVariables.POSTGRES_DB,
synchronize: false, // Should be false in production to use migrations
database: envVariables.POSTGRES_DB,
synchronize: ${env.NODE_ENV === APP_ENVIRONVENT.PRODUCTION}

Copy link

Choose a reason for hiding this comment

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

Typo:
env.NODE_ENV !== APP_ENVIRONVENT.PRODUCTION

Copy link

@rrojan rrojan left a comment

Choose a reason for hiding this comment

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

Small nitpick. Apart from this LGTM 🏆

username: env.POSTGRES_USER,
password: env.POSTGRES_PASSWORD,
database: env.POSTGRES_DB,
synchronize: env.NODE_ENV === APP_ENVIRONVENT.PRODUCTION ? false : true, // Should be false in production to use migrations
Copy link

Choose a reason for hiding this comment

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

Suggested change
synchronize: env.NODE_ENV === APP_ENVIRONVENT.PRODUCTION ? false : true, // Should be false in production to use migrations
synchronize: env.NODE_ENV === APP_ENVIRONVENT.DEVELOPMENT,

arjandhakal
arjandhakal previously approved these changes Dec 31, 2024
Copy link

@rrojan rrojan left a comment

Choose a reason for hiding this comment

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

LGTM 🏆

DEFAULT = 587,
TLS = 465,
}
const validationSchema = z

Choose a reason for hiding this comment

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

validationSchema-> appEnvSchema

.gitignore Outdated
@@ -54,3 +54,5 @@ pids

# Diagnostic reports (https://nodejs.org/api/report.html)
report.[0-9]*.[0-9]*.[0-9]*.[0-9]*.json

ormconfig.json

Choose a reason for hiding this comment

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

should this file be ignored @pratham-outside

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

6 participants