Skip to content

Conversation

gabrielmachin
Copy link
Collaborator

NOTION Ticket

Type of change

  • Fix
  • Story
  • Chore

Description of the change

Used preview-email package to avoid using a SMTP service on development and testing, also update config schema to not need to configure SMTP environment vars on development and testing.

Comment on lines 29 to 30
if (process.env.NODE_ENV === 'production') return !!val;
return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (process.env.NODE_ENV === 'production') return !!val;
return true;
return process.env.NODE_ENV !== 'production' || val;

import { config } from 'config/config';
import { config, isProduction } from 'config/config';

const emailTransporter = nodemailer.createTransport({
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't this also be hidden depending on the environment?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

By hidden do you mean something like this?

Suggested change
const emailTransporter = nodemailer.createTransport({
const emailTransporter = isProduction
? nodemailer.createTransport({
host: config.smtpHost,
port: config.smtpPort,
auth: {
user: config.smtpUser,
pass: config.smtpPassword,
},
})
: null;

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes

.refine((val) => {
if (process.env.NODE_ENV === 'production') return !Number.isNaN(val);
return true;
return process.env.NODE_ENV !== 'production' || val;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would you mind updating all other cases with this? Also, I think we can change this to implicit return

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.

2 participants