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

added SMTP support and removed resend dependency #313

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

Conversation

Chigala
Copy link
Contributor

@Chigala Chigala commented Aug 11, 2023

Description

Added SMTP support

Changes

  • Removed resend
  • Added nodemailer
  • added support for SMTP

fixes #282

@changeset-bot
Copy link

changeset-bot bot commented Aug 11, 2023

⚠️ No Changeset found

Latest commit: 55c5df6

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@ericallam ericallam left a comment

Choose a reason for hiding this comment

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

As well as these changes, we will need to update the following docs

apps/webapp/app/env.server.ts Outdated Show resolved Hide resolved
packages/emails/src/index.tsx Outdated Show resolved Hide resolved
packages/emails/src/index.tsx Outdated Show resolved Hide resolved
@nicktrn
Copy link
Collaborator

nicktrn commented Aug 16, 2023

You may notice some issues with SMTP depending on provider. Might be worth enabling pooling if that happens (yes, that's my code word now).

@Chigala
Copy link
Contributor Author

Chigala commented Aug 16, 2023

Lol, I'll definitely look into this. Thanks

Copy link
Member

@matt-aitken matt-aitken left a comment

Choose a reason for hiding this comment

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

This works! I had to make a small change to coerce the env var string to a number

@matt-aitken
Copy link
Member

@Chigala to merge this that checklist above from @ericallam needs to be completed

@Chigala
Copy link
Contributor Author

Chigala commented Aug 18, 2023

@matt-aitken I've already sent pull requests to the docker and fiy.io repositories.

from: string;
replyTo: string;
}) {
this.#client = config.smtpConfig ? nodemailer.createTransport(config.smtpConfig) : undefined;
Copy link
Member

Choose a reason for hiding this comment

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

this smtpConfig option is always going to be "truthy" so the nodemailer.createTransport thing is always going to be called:

CleanShot 2023-08-21 at 16 14 58

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm sorry but I don't quite understand this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ericallam Should I put back the resend api key and dependency as a fallback to when the smtp config doesn't exist?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think Eric just meant the config values should be checked instead of the entire object. Otherwise, this will never return undefined. Checking for truthy smtpConfig.host should probably be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ohh, makes sense. Thanks

@ericallam
Copy link
Member

@Chigala when you get a chance could you check my comment above?

@gautamsi
Copy link
Contributor

is this planned? I can recreate the PR and update the code if @Chigala is not available

@ghostdevv
Copy link
Contributor

also happy to help work on a revival of this 🙏

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.

[TRI-951] Remove resend dependency from the webapp and replace with nodemailer and smtp
6 participants