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

Upgrade to new Sendgrid NodeJS 4.x (including support for categories) #28

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

glesage
Copy link

@glesage glesage commented Oct 6, 2016

This was referenced Oct 6, 2016
@elboletaire
Copy link

Why the change from two to four spaces?

@glesage
Copy link
Author

glesage commented Oct 14, 2016

hm.. beautifier default I guess, I can revert those to 2 spaces

@glesage
Copy link
Author

glesage commented Oct 14, 2016

Ok done now.

Node: For some reason the sendgrid folks had made the tests 4 space indented, but the code itself 2 space indents... now everything is 2 space indented

@pibi
Copy link

pibi commented Mar 27, 2017

@glesage it would be great if you could publish your fork as a scoped package (https://docs.npmjs.com/getting-started/scoped-packages#publishing-a-scoped-package) on npm. This way we can update our code with your version, while keeping the official version in the hands of the sendgrid folks. What do you think?

@pibi
Copy link

pibi commented Mar 29, 2017

@glesage I tested your code with latest sendgrid and your "simple.js" example. it sends the email, but without the attachment. For reference, here is the email I got:

Content-Type: multipart/alternative; boundary=a695d6af36ee3c70b3499262a0295df31062e27e1fb7c6419bb7947fdccf
Date: Wed, 29 Mar 2017 08:07:05 +0000
From: baz@example.com
Mime-Version: 1.0
To: zzz@zzzzzzz.zzz
Message-ID: <F4q1uhXESiyalj4CssTpZA@ismtpd0004p1lon1.sendgrid.net>
Subject: Hi there
X-SG-EID: HqT4kUctyE3kW2PUSsq91RU8LLKTVuz0GmTxV/aozaQtOMmmETFfcCXs8ttd+SgQq7CBmLntkDeEWg
 kLbsQ30qR5s4SdeZZbjYGhHMQVk+EHmqtbskuwkLTlODISEX0B3fKYbLTuahK70uajTVjffa+02PMF
 oPPcx5Om4PLXg1Vq95Pl044mlhcXZGWvX7rx

--a695d6af36ee3c70b3499262a0295df31062e27e1fb7c6419bb7947fdccf
Content-Transfer-Encoding: quoted-printable
Content-Type: text/plain; charset=UTF-8
Mime-Version: 1.0

Awesome sauce=

--a695d6af36ee3c70b3499262a0295df31062e27e1fb7c6419bb7947fdccf
Content-Transfer-Encoding: quoted-printable
Content-Type: text/html; charset=UTF-8
Mime-Version: 1.0

<b>Awesome sauce</b><img src=3D"https://u5022465.ct.sendgrid.net/wf/open?up=
n=3DnMvfCq7WiY0yYcCX2AtMT-2BrDmRSsjXvx5pKaPAhucVuT8WTPS6aCzTuVw6qd4-2BO9T4G=
nYTGcv0cfbrmlRfGNsRJAYIw85LSqvcPu-2BQbN2MowzRSoxS-2Fl84DPdQB-2FKyf-2BVXmtIX=
TbZ-2FEO-2Br5rPZJ9uTqDsL-2B0AX-2B-2FyEFPtZeymC4wcJsO5-2BVRMc-2FxDzahIlyZZVl=
x4RFjHFsFoJgLRZuHQC89B7ta4pXTw7Mirbhgi-2Fg-3D" alt=3D"" width=3D"1" height=
=3D"1" border=3D"0" style=3D"height:1px !important;width:1px !important;bor=
der-width:0 !important;margin-top:0 !important;margin-bottom:0 !important;m=
argin-right:0 !important;margin-left:0 !important;padding-top:0 !important;=
padding-bottom:0 !important;padding-right:0 !important;padding-left:0 !impo=
rtant;"/>

--a695d6af36ee3c70b3499262a0295df31062e27e1fb7c6419bb7947fdccf--

@glesage
Copy link
Author

glesage commented Apr 3, 2017

Ok @pibi sounds good I'll look into the scoped package situation

As for the attachment issue, I'll try to take a look as well. We don't use attachments at Starchup (platform I built this for) so I probably didn't notice earlier. Wish email clients were unit-testable (:

@dtzar
Copy link

dtzar commented May 7, 2017

Any idea on roughly when you might approve this PR? We could really use it now.

@thinkingserious
Copy link
Contributor

Hi @dtzar,

We are going to revisit re-supporting this library after this PR is accepted: sendgrid/sendgrid-nodejs#378

Thanks!

Elmer

@pibi
Copy link

pibi commented Jul 16, 2017

Any news on this @thinkingserious ? @glesage did you try to publish the module as a scoped package?

@glesage
Copy link
Author

glesage commented Jul 17, 2017

I was actually just looking but am unclear on what the plan is... I see this commit that @thinkingserious mentioned https://github.com/sendgrid/sendgrid-nodejs/pull/378/commits

but it doesn't seem to have anything different with master, or it's contents have already been merged

Unclear whether I still need to do this fork as a scoped package system @pibi ?

@thinkingserious
Copy link
Contributor

HI @pibi,

We are still working on sendgrid/sendgrid-nodejs#378. We are getting closer, but we still have some work to do. Thanks for checking in!

@glesage,

The plan is to first finish the upgrade of our sendgrid-nodejs package, then we will revisit updating this repo. Thanks!

With Best Regards,

Elmer

@dtzar
Copy link

dtzar commented Aug 31, 2017

Bump - 378 is now merged/closed 👍
Any idea of roughly when you might be able to get to this?

@thinkingserious
Copy link
Contributor

Hello Everyone,

Now that sendgrid-nodejs has been updated, please join the conversation over here.

For this issue to gain priority in our backlog, we need additional +1's or a PR. When we receive a PR, that provides the biggest jump in priority.

With Best Regards,

Elmer

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants