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

Return a mocked status when sending emails in test/preview mode. #473

Open
danielthedifficult opened this issue Jul 12, 2023 · 4 comments
Open

Comments

@danielthedifficult
Copy link

danielthedifficult commented Jul 12, 2023

Is your feature request related to a problem? Please describe.
I'm wanting to check that an email was successfully sent (or not) before deleting an item from our notification queue.

In prod/regular sending mode, it returns the .json() of the underlying nodemailer result on line 68:

const sendMailResult = await sendMail({
component,
previewName,
...mailOptions,
html,
});
const json = sendMailResult ? { result: sendMailResult } : {};
return res.status(200).json(json);
} catch (e: any) {
if ("number" === typeof e.status) {
return res.status(e.status).json({ error: e.message });
} else {
return res
.status(500)
.json({ error: `sendMail returned an error: ${e.message}` });
}

Unfortunately, in test/preview mode, the sendMail function doesn't return anything. Maybe this was a simple oversight? Could it be as simple as just adding a return in front of lines 46 and 50?

await sendMail({
component,
to,
dangerouslyForceDeliver: true,
subject,
});
res.json({});
} catch (e: any) {
error("error sending mail", e);
res
.status(500)
.json({ error: "sendMail threw an error: " + jsonStringifyError(e) });
}

Or maybe I'm confused and these are actually the relevant lines:

export function createIntercept(req: IncomingMessage, res: ServerResponse) {
let body = "";
req.on("data", function onData(data) {
body += data;
if (body.length > 1e8) req.destroy();
});
req.on("end", function onEnd() {
const id = randomUUID();
cache[id] = { ...JSON.parse(body), id };
res.writeHead(201);
res.end(JSON.stringify({ id }));
log(`Cached intercept preview at /previews/${id}`);
});

UPDATE:

I just tried, on a hunch, some .then/.catch syntax and it works:

    await sendMail({
      to: someEmail,
      component: <MyEmailTemplate />,
    })
      .then(() => {console.log('RESOLVED')})
      .catch(() => console.log('FAILED'));

logs "RESOLVED" when testing (even with no to: email address) and "FAILED" if I don't provide a component prop.

Maybe that's GEFN?

Describe the solution you'd like
I think returning a mocked status that closely resembles what nodemailer would give, i.e. a successful promise with perhaps an auto-generated message ID, 200 status code, etc., would be best.

@psugihara
Copy link
Collaborator

Heya, sorry for slowness on the response. Deprioritized when I saw your workaround then forgot 🙃

I believe this is the block returning undefined:

if (testMode && !dangerouslyForceDeliver) {
const testMessageQueue = await getTestMailQueue();
testMessageQueue.push(mailOptions);
await fs.writeFile(TMP_TEST_FILE, JSON.stringify(testMessageQueue));
return;
} else if (previewMode) {
await openPreview(mail, mailOptions, previewServerUrl);
return;
}

Your workaround seems good if that works for you. The difficulty with this feature would be that different transports will have different response types. I'm open to just making a generic response (e.g. {} rather than undefined) but I worry that going too much more complicated would result in confusion when looking at the sendMail response in dev vs prod.

I'd opt to close if the workaround is good enough but lmk.

@danielthedifficult
Copy link
Author

I think returning an empty object is a good start. I am confused about how standard a "transport" is, and whether or not they all return the SMTP stats codes, etc.

But, again, with this workaround, a bit of documentation, and returning an empty object that's probably enough.

@psugihara
Copy link
Collaborator

Found the doc on transport return types here: https://nodemailer.com/plugins/create/#transports

Looks like the common return type includes envelope and messageId. For the 3 built-in nodemailer transports they look like this:

envelope – is an envelope object {from:‘address’, to:[‘address’]}
messageId – is the Message-ID header value. This value is derived from the response of SES API, so it differs from the Message-ID values used in logging.

I'd be open to just use that as a spec for a dummy return value.

Of course, there are no promises that 3rd party nodemailer transports (e.g. the mailgun one) conform to this.

@danielthedifficult
Copy link
Author

That's perfect, exactly what I was hoping. Nice find.

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

No branches or pull requests

2 participants