Email package splits lines incorrectly / upgrade mailcomposer

[edemaine]

Try this on Meteor server (without MAIL_URL set, so that it prints to stdout):

Email.send({
  subject: "This is a very long email subject to illustrate how it gets split onto multiple lines"
})

The result is this:

I20170224-21:23:03.030(-5)? ====== BEGIN MAIL #0 ======
I20170224-21:23:03.030(-5)? (Mail not sent; to enable sending, set the MAIL_URL environment variable.)
I20170224-21:23:03.031(-5)? MIME-Version: 1.0
I20170224-21:23:03.031(-5)? Date: Sat, 25 Feb 2017 02:23:03 +0000
I20170224-21:23:03.031(-5)? Subject: This is a very long email subject to illustrate how it gets split
I20170224-21:23:03.031(-5)?         onto multiple lines
I20170224-21:23:03.032(-5)? Content-Type: text/plain; charset=utf-8
I20170224-21:23:03.032(-5)? Content-Transfer-Encoding: quoted-printable
I20170224-21:23:03.032(-5)?
I20170224-21:23:03.032(-5)?
I20170224-21:23:03.033(-5)?
I20170224-21:23:03.033(-5)? ====== END MAIL #0 ======

Unfortunately, this behavior seems incorrect. Specifically, there are 8 spaces before "onto multiple lines", but there should be only one. According to RFC822, "Unfolding is accomplished by regarding CRLF immediately followed by a LWSP-char as equivalent to the LWSP-char." So while I only asked for one space before "onto" in the subject, I got 8 instead.

Specifically, this results in an actual error when setting MAIL_URL, producing actual email, and reading it in an email client. At least in Pine, the subject gets eight spaces in the middle, which looks rather silly. I don't know for sure that it will render incorrectly in all email clients, but it certainly isn't following the RFC822 spec.

I tried to reproduce this error in node using mailcomposer, but it seems to implement RFC822 correctly:

var MailComposer = require('mailcomposer')
var mc = new MailComposer()
mc.addHeader({subject: "This is a very long email subject to illustrate how it gets split onto multiple lines"})
mc.buildHeaders()

produces:

'Content-Type: text/plain\r\nMessage-ID: <67ed00a7-8695-b5b7-261b-af9d3b033ed5@ibis>\r\nSubject: This is a very long email subject to illustrate how it gets split\r\n onto multiple lines\r\nDate: Sat, 25 Feb 2017 02:31:57 +0000\r\nMIME-Version: 1.0'

I couldn't get Mailcomposer 0.1.15 (as used by Meteor) to work at all in Node. Presumably the bug got fixed between 0.1.15 and the latest version, which is 4.0.1 (!). Could we please upgrade?

[abernix]

Thanks for reporting this!

It would be amazing to get mailcomposer upgraded (especially if it would fix this for you) but this isn't high on the Roadmap right now so a community contribution would be awesome! Would you be interested in helping make a contribution to Meteor by tackling this and submitting a pull-request?

For starters, it would involve bumping the version in packages/email/package.js and obviously some testing, especially of the accounts* packages.

Let us know if this is something you could look into!

EDIT: I just saw the comment about why the version is pinned to such an old version. Still should be investigated.

// Pinned at older version. 0.1.16+ uses mimelib, not mimelib-noiconv which is
// much bigger. We need a better solution.

[abernix]

I've marked this as pull-requests-encouraged so if you can't look at it, hopefully someone can. While mailcomposer seems to work for sending in general, the version gap is definitely substantial enough that it'd be nice to get that fixed ASAP. 😉

[laosb]

Shall we just update the mailcomposer to 4.0.1? Is there any specific API changes that need to be applied to email package?

PS: #6267 sounds related to this too.

[abernix]

@laosb See my comment above. There's a reason why it's pinned and it needs to be investigated.

Is there any specific API changes that need to be applied to email package?

Probably! But that's part of investigating. 😉

[edemaine]

My sense is that, while the API may have changed, the Email package uses so little of the API that it's probably fine to just upgrade. Just needs to be tested. I haven't modified Meteor before, so I'm not familiar yet with the testing harness, so if others have time feel free; otherwise, I will try to look at this.

[abernix]

Agree on the API.

I haven't validated whether this can be reduced (maybe we can just remove some of dependency?), but the increase is ~500KB. It's a server-only package, so it's not a huge impact to the client, but it is a large increase. Given this factor, it'd be reasonable to first consider an alternate package to use in its place, assuming there is something compatible.

[jykae]

Nodemailer https://nodemailer.com is quite popular if considering another package.

[edemaine]

Nodemailer is actually what replaced mailcomposer (at least according to mailcomposer's Github which is linked from NPM package mailcomposer). However, I'm not sure that EUPL 1.1 code is compatible with Meteor's MIT license, but I don't quite understand how NPM-included packages work in this regard... Any experts?

Nodemailer v3+ is licensed under the European Union Public License 1.1. Previous versions of Nodemailer are licensed under the MIT license. [https://nodemailer.com/about/license/]

NPM package mailcomposer still lists that part as MIT licensed...

[edemaine]

Inspired by the mimelib vs. mimelib-noiconv comment, I found nodemailer-noiconv, which is an old (0.2.4, 4 years old) version of nodemailer that seems to be smaller (176K) than the mailcomposer@0.1.15 currently in Meteor (268K). Unfortunately, it seems to have the same leading-whitespace bug. (Test code with this API: var nm=require('nodemailer-noiconv'); var e = new nm.EmailMessage({ subject: '...really long...' }); e.generateHeaders();)

In fact, the root issue is with mimelib-noiconv: require('mimelib-noiconv').foldLine('Subject: ...long...') produces the space issue, while require('mimelib').foldLines('Subject: ...long...') does not. Alas, mimelib is 500K larger than mimelib-noiconv (I think because of encoding tables).

[jykae]

@edemaine EUPL v1.1 compatibility https://joinup.ec.europa.eu/software/page/eupl/eupl-compatible-open-source-licences

Hmh, MIT is compatible distributed under EUPL v1.1, but not distributed under MIT, as far as I understand that.

How much other packages in Meteor actually depend on this? Could email package be just recommended for users in Meteor guide for example? I understood that Meteor wants to go more npm way in future, is that correct?

Community nodemailer (aka "nodenvelope") is though MIT-compatible, https://github.com/nodenvelope/nodenvelope

[edemaine]

Licencing: nodemailer overs no advantages over mailcomposer, as they share most of the codebase, and the latter offers the API we want and an MIT license. Thanks for pointing out nodenvelope, but it is alas double the size of mailcomposer. (Both, at their core, just use NPM package buildmail; mailcomposer is roughly the same size as buildmail, while nodenvelope is much larger.)

Dropping email package: I like the email package in the Meteor context as it makes it easy for server configuration via the MAIL_URL environment variable to specify the SMTP server, with a fallback to printing text, instead of writing code to open connection to server and manually fallback to text output. No NPM package I've seen does this. Furthermore, asynchronous calls to remote servers are difficult to do in Meteor, so I don't think it'd be easy to use an NPM package in place of email (though of course Meteor's email could change to an NPM package).

My conclusion of the feasible options:

1. Upgrade to a newer mailcomposer
2. Roll our own from scratch

The first choice is IMO reasonable, as email isn't a default Meteor package, so users can still make the choice about the size increase, and there doesn't seem a smaller alternative. The second choice is feasible --- fundamentally, email isn't that hard to generate --- but obviously more work. @abernix, let me know your thoughts.

[abernix]

@edemaine Thanks for the analysis. As you said, the email package certainly offers benefits and I think we'd like to keep it around. It sounds like updating mailcomposer to the latest version is the way to go here: any Kb weight here will be server-side and an MIT license would be preferred over the EUPL (which I'm still not clear on, though "I am not a lawyer" certainly applies here. Alas.)

My vote here would be to move forward – for starters, by bumping the mailcomposer version and seeing what happens with existing tests.

[edemaine]

I've mostly finished this PR, including a bunch of new test cases, all now passing. But there is a catch: the simplesmtp library we use to actually send email (not represented in the tests) relies on the old (0.1) mailcomposer API for getting the message as a stream, and sadly it is deprecated. simplesmtp recommends migrating to smtp-connection, but that is distributed under EUPL... (All of these packages are written by the same person. Perhaps I should write to him...)

[edemaine]

One option is to use smtp-connection@3.2.0 which is licensed under MIT...
Adds yet another 500K (because I don't see how to get smtp-connection and mailcomposer to share the nodemailer-shared package... maybe some versions match up).

This might be an actual reason to switch from mailcomposer to Nodemailer (capped at version 3 to get MIT license), so that we can use a consistent version of packages. smtp-connection is part of Nodemailer. Instead of mailcomposer, we'd need to use Nodemailer's buildmail directly I think...

Oh, actually, Nodemailer@3 defines MailComposer objects, and rings in at a record 604K! I'll give this a try; hopefully it will suffice and be a best of all worlds. We still won't be able to track to v4 because of license, but v3 seems be maintained as well -- v3.1.7 was released 20 minutes ago! Sorry, Nodemailer@3 is EUPL. We'd need to use Nodemailer@2, which was last updated January (so still recent) but weighs in at 2MB, which is 500K larger than mailcomposer + smtp-connection@3.

OK, I found that mailcomposer (latest, @4) + smtp-connection@2 lets them share nodemailer-shared (@1.1.0) and nodemailer-fetch (@1.6.0), reducing size from 1636K to 1460K. So I'll continue with this choice.

[jykae]

Good deep dive @edemaine 👍 If you are able to combine some elegant & easy to use "email lite" version that is also MIT compatible stuff, maybe it would be worth to package it to npm as well?

[edemaine]

PR complete: #8495