Skip to content

Conversation

@MikeRalphson
Copy link
Contributor

Refs #316

I wanted to implement this as I thought I should have working tests before looking at any other issues.

Maybe @tsriram and I could collaborate on this?

Current limitations; does not work on Node 0.10 or 0.12, which are both at or near end-of-life... (as a bonus, it does pass on Node 6, as well as 4 and iojs).

Please let me know if I'm working along the right lines.

@thinkingserious
Copy link
Contributor

@MikeRalphson,

I'm not opposed to a collaboration, but it looks like @tsriram is out until the 23rd.

In the mean time, we will need a signed CLA in order to merge your changes. Could you please take care of that for us?

@thinkingserious thinkingserious added type: community enhancement feature request not on Twilio's roadmap status: cla needed status: work in progress Twilio or the community is in the process of implementing labels Oct 17, 2016
@MikeRalphson
Copy link
Contributor Author

@thinkingserious CLA DocuSign'ed and sent.

@MikeRalphson
Copy link
Contributor Author

Now passing on Node 0.12 - PR submitted to system-sleep module, ugly temporary work-around in place for Travis builds.

@thinkingserious what would be the chance of upping the minimum-required Nodejs version to v0.11.12 ? This provides child-process.execSync. If not, I will look at using async-queue...

@thinkingserious
Copy link
Contributor

Based on this table, it looks like we need to keep support for .10 until 12/31/16.

Do you have a reference that could help us make the justification to drop support earlier?

Thanks!

@MikeRalphson
Copy link
Contributor Author

@thinkingserious as per the table you quoted, I believe the original plan was to end LTS for Node 0.10 three months before the OpenSSL end-of-life of 31-Dec-2016, i.e. 01-Oct-2016 . This has now crept out to the end of October.

@thinkingserious
Copy link
Contributor

Thanks @MikeRalphson!

We will remove support on November 1 in that case.

@heitortsergent
Copy link
Contributor

@MikeRalphson did you run into this error when running the tests by any chance?

...
Uncaught SyntaxError: Unexpected token M in JSON at position 0
      at JSON.parse (<anonymous>)
      at lib/sendgrid.js:110:42
...

I was testing out the changes to see if everything was working with Node.js v7. 😄

@MikeRalphson
Copy link
Contributor Author

MikeRalphson commented Nov 1, 2016

@heitortsergent No all tests passed on Node 0.12, 4 and 6. I will try and add Node 7 and see if the error appears for me.

Update: all tests pass on Node 7 too.

@heitortsergent
Copy link
Contributor

@MikeRalphson that's great!

I think the error I'm seeing might be a local problem then... Do I need to run any command(s) besides npm install before running npm test?

@MikeRalphson
Copy link
Contributor Author

@heitortsergent You shouldn't need to, no.

Maybe try replacing the two instances of

response.body = response.body ? JSON.parse(response.body) : response.body;

in lib/sendgrid.js with

try {
    response.body = JSON.parse(response.body);
}
catch (ex) {}

To help diagnose the issue?

@thinkingserious
Copy link
Contributor

Hi @MikeRalphson,

Once the conflicts are merged, is this one ready to go?

With Best Regards,

Elmer

@SendGridDX
Copy link

SendGridDX commented May 3, 2017

CLA assistant check
Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ thinkingserious
❌ MikeRalphson
You have signed the CLA already but the status is still pending? Let us recheck it.

@thinkingserious thinkingserious merged commit d40d706 into sendgrid:master May 3, 2017
@thinkingserious
Copy link
Contributor

Hello @MikeRalphson,

Thanks again for the PR!

We appreciate your contribution and look forward to continued collaboration. Thanks!

Team SendGrid DX

thinkingserious added a commit that referenced this pull request May 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status: work in progress Twilio or the community is in the process of implementing type: community enhancement feature request not on Twilio's roadmap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants