WIP: Add std/smtp module #8413

uki00a · 2020-11-17T13:10:55Z

Towards #7906.

Summary

This PR adds std/smtp module.
The implementation and API are based on the Golang's net/smtp package.

Usage

import { sendMail } from "https://deno.land/std/smtp/mod.ts";

await sendMail({
  addr,
  from: "from@example.com",
  to: ["to@example.com"],
  msg
});

Changes

Add std/smtp module.
Move TextProtoReader class from std/textproto/mod.ts to std/textproto/reader.ts and re-export it from std/textproto/mod.ts.
Add TextProtoWriter class (std/textproto/writer.ts)
Add TextProtoConn class (std/textproto/conn.ts)

Question

This PR is a bit more complex 😢 Should I split this PR into multiple PRs?

TODO

Add more tests
Cleanup code
Fix implementation of cram-md5 (which is currently broken)
Add README.md

rheh · 2020-11-17T20:21:22Z

std/smtp/_helpers.ts

+  * Use of this source code is governed by a BSD-style
+  * license that can be found in the LICENSE file.
+  */
+export function validateLine(line: string): void {


Maybe move into _address_validation.ts to remove the generically named helpers.ts?

_util.ts would be more consistent. These are private, I wouldn't worry about a generic _util module per-directory.

rheh · 2020-11-17T20:22:27Z

std/smtp/_helpers.ts

+ * See func Dial for a description of the hostport parameter, and host
+ * and port results.
+ */
+export function splitHostPort(hostport: string): [string, number] {


Maybe move into _address_parameter_extrator.ts to remove the generically named helpers.ts?

rheh · 2020-11-17T20:23:40Z

std/smtp/_helpers.ts

+export function splitHostPort(hostport: string): [string, number] {
+  const missingPort = "missing port in address";
+  const tooManyColons = "too many colons in address";
+  function addrErr(addr: string, why: string): AddrError {


Avoid contraction and use addressError as the name?

rheh · 2020-11-17T20:26:11Z

std/smtp/_helpers.ts

+  let j = 0, k = 0;
+
+  // The port starts after the last colon.
+  const i = hostport.lastIndexOf(":");


nit pick: Not a fan of variable name with single letter, how about indexOfLastColon

rheh · 2020-11-17T20:26:30Z

std/smtp/_helpers.ts

+  function addrErr(addr: string, why: string): AddrError {
+    return new AddrError(why, addr);
+  }
+  let j = 0, k = 0;


nit pick: Not a fan or single letter variables

rheh · 2020-11-17T20:50:02Z

std/textproto/reader.ts

+      let code2: number;
+      let moreMessage: string;
+      [code2, continued, moreMessage] = await parseCodeLine(line, 0);
+      if (code2 !== code) {


code2 and code are hard to understand as variable names? Can these be named better?

rheh · 2020-11-17T20:50:40Z

std/textproto/reader.ts

+        continued = true;
+        continue;
+      }
+      message += "\n" + moreMessage;


Add a named constant for \n?

rheh · 2020-11-17T20:51:15Z

std/textproto/reader.ts

+  skipSpace(l: Uint8Array): number {
+    let n = 0;
+    for (let i = 0; i < l.length; i++) {
+      if (l[i] === charCode(" ") || l[i] === charCode("\t")) {


Again, would it be more reabable to name named constants for space and tab?

rheh · 2020-11-17T20:52:22Z

std/textproto/reader.ts

+}
+
+function parseCodeLine(line: string, expectCode: number): CodeLine {
+  if (line.length < 4 || (line[3] !== " " && line[3] !== "-")) {


A few magic numbers here. Named constant for 3 and 4 would reveal the position significance?

rheh · 2020-11-17T20:55:01Z

std/textproto/writer.ts

+          break;
+        case WRITE_STATE_CR:
+          this.#state = WRITE_STATE_DATA;
+          if (c === charCode("\n")) {


More uses of the \n and \r. As these are also reference in the other files can they share a enum to improve readability

nayeemrmn · 2020-11-17T21:56:08Z

std/smtp/_helpers.ts

+class AddrError extends Error {
+  constructor(why: string, readonly addr: string) {
+    super(why);
+  }
+}


This needs to be exported if users see it. Does this class need to exist, though?

kitsonk · 2020-11-18T04:23:58Z

Please not a tag of a suggestion as the original issue is tagged does not mean there is agreement to add something. It is simply a request. We should really resolve if it is suitable in the issue before PRs are raised.

I personally believe we shouldn't add something like this to std. It is a great opportunity for it to be a community module.

ry · 2020-11-19T12:14:07Z

@uki00a This looks pretty good! But because of the size - it's going to be difficult for us to review this properly.

I would feel much more comfortable merging this into std if it first existed in deno.land/x people were using it.

How does this compare to? https://deno.land/x/smtp@v0.6.0

uki00a · 2020-11-19T13:00:39Z

@ry From my personal point of view, I compared each module:

std/smtp

Provides both high-level (sendMail()) and low-level (SmtpClient#mail(), SmtpClient#rcpt(), SmtpClient#data(), etc.) APIs for sending mails.

x/smtp

Published since 2019/06 (so some users may already be using this module).
Provides high-level API (SmtpClient#send()) for sending emails.
The author, @manyuanrong, has made significant contributions to the Deno community (e.g. deno_mysql and deno_mongo), so I consider this module to be reliable as well.

My personal thoughts

x/smtp looks good to me 👍
I think it may be confusing that SMTP modules exist in both /std and /x.

stale · 2021-01-21T05:15:04Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

bartlomieju · 2021-01-25T16:26:20Z

@uki00a looks like this PR is still not ready, so we won't land this PR before standard library migration to a separate repository (#9029 (comment)).

I'm still very much interested in discussing these changes on upcoming meetings and potentially landing the PR.

PodaruDragos · 2021-01-25T21:55:03Z

this would be such a good addition to std. i know people disagree with the fact that the smtp module should be part of std, but look at node and nodemailer and after then maybe reconsider :)

bartlomieju · 2021-02-01T12:04:55Z

Deno standard library was moved to denoland/deno_std, please reopen your PR there.

uki00a changed the title ~~WIP: add std/smtp module~~ WIP: Add std/smtp module Nov 17, 2020

rheh reviewed Nov 17, 2020

View reviewed changes

nayeemrmn reviewed Nov 17, 2020

View reviewed changes

bartlomieju mentioned this pull request Nov 21, 2020

feat(std): add mail parsing to std/mail #7925

Closed

2 tasks

stale bot added the stale label Jan 21, 2021

stale bot removed the stale label Jan 25, 2021

bartlomieju added the std_migration label Jan 25, 2021

uki00a added 6 commits January 31, 2021 23:42

WIP: add std/smtp module

30b77a5

test: Add 'sendMail with AUTH' test

4f484e3

test: Added 'auth failed' case

2099988

Fix

d4f58de

chore: Lint and format files

a4614cc

chore: Fmt

7d00044

bartlomieju closed this Feb 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Add std/smtp module #8413

WIP: Add std/smtp module #8413

uki00a commented Nov 17, 2020 •

edited

Loading

rheh Nov 17, 2020

nayeemrmn Nov 17, 2020

rheh Nov 17, 2020

rheh Nov 17, 2020

rheh Nov 17, 2020

rheh Nov 17, 2020

rheh Nov 17, 2020

rheh Nov 17, 2020

rheh Nov 17, 2020

rheh Nov 17, 2020

rheh Nov 17, 2020

nayeemrmn Nov 17, 2020

kitsonk commented Nov 18, 2020

ry commented Nov 19, 2020

uki00a commented Nov 19, 2020 •

edited

Loading

stale bot commented Jan 21, 2021

bartlomieju commented Jan 25, 2021

PodaruDragos commented Jan 25, 2021 •

edited

Loading

bartlomieju commented Feb 1, 2021

WIP: Add std/smtp module #8413

WIP: Add std/smtp module #8413

Conversation

uki00a commented Nov 17, 2020 • edited Loading

Summary

Usage

Changes

Question

TODO

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kitsonk commented Nov 18, 2020

ry commented Nov 19, 2020

uki00a commented Nov 19, 2020 • edited Loading

std/smtp

My personal thoughts

stale bot commented Jan 21, 2021

bartlomieju commented Jan 25, 2021

PodaruDragos commented Jan 25, 2021 • edited Loading

bartlomieju commented Feb 1, 2021

uki00a commented Nov 17, 2020 •

edited

Loading

uki00a commented Nov 19, 2020 •

edited

Loading

PodaruDragos commented Jan 25, 2021 •

edited

Loading