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

Switch to modern nodemailer 4, Node 4 version. Fix #8591 #8605

Merged
merged 4 commits into from Apr 17, 2017

Conversation

Projects
None yet
3 participants
@edemaine
Contributor

edemaine commented Apr 16, 2017

  • Most critically, use a pool instead of direct SMTP connection, to handle dropped connections and increase throughput, like mail module 1.1. (#8591)
  • New nodemailer's sendMail wants an options object, not a MailComposer object. Luckily, a MailComposer object has a "mail" field that remembers the original options, so we can keep original behavior.
  • However, we no longer support the mailComposer option set to a compiled MailComposer object (functionality that was briefly added in 1.2.0).
  • nodemailer does SMTP URL parsing now automatically for us, simplifying code.
  • Tests' outputs now end with additional "\r\n"
  • Drop underscore package dependency (no longer needed)

edemaine and others added some commits Apr 16, 2017

Switch to modern nodemailer 4, Node 4 version. Fix #8591
* Most critically, use a pool instead of direct SMTP connection,
  to handle dropped connections and increase throughput,
  like mail module 1.1.  (#8591)
* New nodemailer's sendMail wants an options object, not a MailComposer
  object.  Luckily, a MailComposer object has a "mail" field that
  remembers the original options, so we can keep original behavior.
* However, we no longer support the mailComposer option set to a compiled
  MailComposer object (functionality that was briefly added in 1.2.0).
* nodemailer does SMTP URL parsing now automatically for us, simplifying code.
* Tests' outputs now end with additional "\r\n"
* Drop underscore package dependency (no longer needed)
@abernix

This comment has been minimized.

Member

abernix commented Apr 17, 2017

Just an FYI! This is a relatively easy situation to have happen with Git submodules (which we use), but this PR had the blaze submodule pointing to a different point than the devel branch. This is the explanation for the test failures on both CI providers and these additional "changes" which were appearing at the bottom of the Diff.

To rectify this (I'd like to look at this early this coming week!), I've merged the most recent devel in and updated the submodule to the correct ref and pushed the changes to this PR which re-triggered the CI builds. Hopefully they pass now (so far so good!)

In the future, in addition to the usual syncing a fork, it should just be a matter of running:

git submodule update --init --recursive

...if you ever notice the following in git status:

modified:   packages/non-core/blaze (untracked content)

😄

@engelgabriel

This comment has been minimized.

Contributor

engelgabriel commented Apr 17, 2017

We are waiting on this to close:
RocketChat/Rocket.Chat#6705

@edemaine

This comment has been minimized.

Contributor

edemaine commented Apr 17, 2017

@abernix Thanks!! I was confused about that; your explanation makes sense. Thanks for both fixing it and explaining so I don't make the mistake in the future.

@abernix abernix self-assigned this Apr 17, 2017

General formatting/style cleanup for `packages/email`.
* snake_cased => camelCased for some local variables.
* Added curly-brackets to `if`s.
* Removed trailing spaces.
* Removed commented-out code.
* Removed older doc text and changed some links.

@abernix abernix changed the base branch from devel to master Apr 17, 2017

@abernix abernix changed the base branch from master to devel Apr 17, 2017

@edemaine

This comment has been minimized.

Contributor

edemaine commented on 87907e3 Apr 17, 2017

Nice cleanup, thanks! (Much of this was not "my" changed code, but nice to see the code cleaned up in any case.)

@abernix abernix merged commit 6d45626 into meteor:devel Apr 17, 2017

1 of 3 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
CLA Author has signed the Meteor CLA.
Details

abernix added a commit that referenced this pull request Apr 17, 2017

Switch to modern nodemailer 4, Node 4 version. Fix #8591 (#8605)
* Switch to modern nodemailer 4, Node 4 version. Fix #8591

* Most critically, use a pool instead of direct SMTP connection,
  to handle dropped connections and increase throughput,
  like mail module 1.1.  (#8591)
* New nodemailer's sendMail wants an options object, not a MailComposer
  object.  Luckily, a MailComposer object has a "mail" field that
  remembers the original options, so we can keep original behavior.
* However, we no longer support the mailComposer option set to a compiled
  MailComposer object (functionality that was briefly added in 1.2.0).
* nodemailer does SMTP URL parsing now automatically for us, simplifying code.
* Tests' outputs now end with additional "\r\n"
* Drop underscore package dependency (no longer needed)

* General formatting/style cleanup for `packages/email`.

* snake_cased => camelCased for some local variables.
* Added curly-brackets to `if`s.
* Removed trailing spaces.
* Removed commented-out code.
* Removed older doc text and changed some links.

* Get rid of back-and-forth assigning of `mailUrlString`.

@abernix abernix referenced this pull request Apr 17, 2017

Merged

email@1.2.1 #8609

@abernix

This comment has been minimized.

Member

abernix commented Apr 17, 2017

Yes, most of these were just general clean-up, certainly not specific to any of your additions/changes, but made sense to do while testing/reviewing it already. 😉

abernix added a commit that referenced this pull request Apr 25, 2017

Switch to modern nodemailer 4, Node 4 version. Fix #8591 (#8605)
* Switch to modern nodemailer 4, Node 4 version. Fix #8591

* Most critically, use a pool instead of direct SMTP connection,
  to handle dropped connections and increase throughput,
  like mail module 1.1.  (#8591)
* New nodemailer's sendMail wants an options object, not a MailComposer
  object.  Luckily, a MailComposer object has a "mail" field that
  remembers the original options, so we can keep original behavior.
* However, we no longer support the mailComposer option set to a compiled
  MailComposer object (functionality that was briefly added in 1.2.0).
* nodemailer does SMTP URL parsing now automatically for us, simplifying code.
* Tests' outputs now end with additional "\r\n"
* Drop underscore package dependency (no longer needed)

* General formatting/style cleanup for `packages/email`.

* snake_cased => camelCased for some local variables.
* Added curly-brackets to `if`s.
* Removed trailing spaces.
* Removed commented-out code.
* Removed older doc text and changed some links.

* Get rid of back-and-forth assigning of `mailUrlString`.

abernix added a commit that referenced this pull request Apr 25, 2017

Switch to modern nodemailer 4, Node 4 version. Fix #8591 (#8605)
* Switch to modern nodemailer 4, Node 4 version. Fix #8591

* Most critically, use a pool instead of direct SMTP connection,
  to handle dropped connections and increase throughput,
  like mail module 1.1.  (#8591)
* New nodemailer's sendMail wants an options object, not a MailComposer
  object.  Luckily, a MailComposer object has a "mail" field that
  remembers the original options, so we can keep original behavior.
* However, we no longer support the mailComposer option set to a compiled
  MailComposer object (functionality that was briefly added in 1.2.0).
* nodemailer does SMTP URL parsing now automatically for us, simplifying code.
* Tests' outputs now end with additional "\r\n"
* Drop underscore package dependency (no longer needed)

* General formatting/style cleanup for `packages/email`.

* snake_cased => camelCased for some local variables.
* Added curly-brackets to `if`s.
* Removed trailing spaces.
* Removed commented-out code.
* Removed older doc text and changed some links.

* Get rid of back-and-forth assigning of `mailUrlString`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment