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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from
Apr 17, 2017
Merged

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

merged 4 commits into from
Apr 17, 2017

Conversation

edemaine
Copy link
Contributor

  • Most critically, use a pool instead of direct SMTP connection, to handle dropped connections and increase throughput, like mail module 1.1. (Meteor email keeps SMTP connection opened and causes error with some email providers #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 2 commits April 15, 2017 23:21
@edemaine
Switch to modern nodemailer 4, Node 4 version. Fix meteor#8591
741cc4c 
* Most critically, use a pool instead of direct SMTP connection,
  to handle dropped connections and increase throughput,
  like mail module 1.1.  (meteor#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
Merge branch 'devel' into pulls/8605
98f6935
@abernix
Copy link
Contributor

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)

😄

@abernix abernix mentioned this pull request Apr 17, 2017
Closed
@engelgabriel engelgabriel mentioned this pull request Apr 17, 2017
Closed
@engelgabriel
Copy link
Contributor

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

@edemaine
Copy link
Contributor Author

edemaine commented Apr 17, 2017 •
edited
Loading

@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
@abernix
General formatting/style cleanup for packages/email.
87907e3 
* 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 April 17, 2017 18:44
@abernix abernix changed the base branch from master to devel April 17, 2017 18:45
@abernix abernix merged commit 6d45626 into meteor:devel Apr 17, 2017
abernix pushed a commit that referenced this pull request Apr 17, 2017
@edemaine @abernix
Switch to modern nodemailer 4, Node 4 version. Fix #8591 (#8605)
57b050a 
* 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 mentioned this pull request Apr 17, 2017
Merged
@abernix
Copy link
Contributor

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 abernix mentioned this pull request Apr 17, 2017
Closed
abernix pushed a commit that referenced this pull request Apr 25, 2017
@edemaine @abernix
Switch to modern nodemailer 4, Node 4 version. Fix #8591 (#8605)
78c6ac9 
* 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 pushed a commit that referenced this pull request Apr 25, 2017
@edemaine @abernix
Switch to modern nodemailer 4, Node 4 version. Fix #8591 (#8605)
beb6009 
* 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`.
@mgscreativa mgscreativa mentioned this pull request May 15, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abernix abernix abernix approved these changes

Assignees

@abernix abernix

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@edemaine @abernix @engelgabriel