Promise API and consistent callback function signature #261

adamreisnz · 2016-07-26T22:55:13Z

This PR addresses the following issues:

Notable changes:

Extracted some logic into helpers
Using a getEmptyRequest helper to avoid code duplication
emtpyRequest now accepts an object with data to extend the empty request with, this will allow simpler syntax for initializing requests.
Callback function now receives two parameters as per Node conventions (error, response)
If no callback provided, the method will return a promise instead.
Implemented promise API when not passing a callback function
Using native Promise by default if present, but allow users to override this with any other implementation by setting Sendgrid.Promise to any value, e.g. Sendgrid.Promise = require('bluebird')

Example usage:

let sendgrid = require('sendgrid');
sendgrid.SendGrid.Promise = require('bluebird');

let sg = sendgrid.SendGrid(process.env.SENDGRID_API_KEY);
let request = sg.emptyRequest({
  method: 'POST',
  path: '/v3/mail/send',
  body: mail.toJSON()
});

//Send request (returns promise)
sg.API(request)
  .then(response => ...)
  .catch(error => ...);

//Send request (using callback)
sg.API(request, (error, response) => {
  if (error) {
    //boo
  }
  //response always populated, even with error
});

adamreisnz · 2016-07-26T23:36:42Z

@thinkingserious can you check why the build is failing? I haven't made any API changes in this PR yet, so everything should still be working, yet all the tests seem to fail on an incorrect status code.

thinkingserious · 2016-07-26T23:44:50Z

Ah, since you are not part of the SendGrid repo, Travis is not loading the MOCK_HOST variable. I'll send instructions on how to create the test environment locally.

adamreisnz · 2016-07-26T23:51:17Z

Yes I figured that's why the tests would be failing locally, but on Travis it should still work, no?

thinkingserious · 2016-07-27T00:54:50Z

To test locally:

Install Prism: https://stoplight.io/prism
Run ./prism run --mock --list --spec https://raw.githubusercontent.com/sendgrid/sendgrid-oai/master/oai_stoplight.json

This will set up a mock local SendGrid server. When you run your tests locally they should pass.

For Node.js, we have have not automated this yet on Travis (it's coming), so it relies on their hosted server, whose unique URL is hidden behind the MOCK_HOST variable. Travis will only load that variable for those in the sengrid repo.

adamreisnz · 2016-07-27T00:59:25Z

Gotcha. I have added the changes to the API here. I might actually have to update the tests, as the callback now receives parameters in different order as per Node conventions.

Notable changes:

Extracted some logic into helpers
Use getEmptyRequest helper to avoid duplication
Use native Promise by default if present, but allow users to override this with any other implementation
Callback now receives two parameters as per Node conventions (error, response)
If no callback provided, the method will return a promise instead.
Removed unused ESLint plugins from package.json
emtpyRequest now accepts an object with data to extend the empty request with, this will allow simpler syntax for initializing requests.

Example usage:

//Build request
let request = sg.emptyRequest({
  method: 'POST',
  path: '/v3/mail/send',
  body: mail.toJSON()
});

//Send request (returns promise)
sg.API(request)
  .then(response => ...)
  .catch(error => ...);

//Send request (using callback)
sg.API(request, (error, response) => {
  if (error) {
    //boo
  }
  //response always populated, even with error
});

thinkingserious · 2016-07-27T01:04:19Z

This is awesome!

Can you please allow us to send you some swag? Just email [email protected] with a mailing address and T-shirt size.

adamreisnz · 2016-07-27T01:09:44Z

Sure will do, thanks :)

I've just pushed another commit which updates the tests and examples to use the new function signature. As this technically is a breaking change from the current version, you might want to release this under a new major version.

However, as it could also be considered a "fix" to adhere to Node standards, maybe you can get away with a minor release as well. But I'll leave that up to you :)

Let me know if there's anything else.

adamreisnz · 2016-07-27T01:29:26Z

Have run the tests locally with Prism and all pass 👍

thinkingserious · 2016-07-27T15:55:01Z

Awesome, love it!

I've gotten this into our backlog for review and merge.

Unfortunately, we have quite a few tickets in front of it, but this issue can move up our queue if others +1 or add comments.

adamreisnz · 2016-07-27T19:14:14Z

@thinkingserious I've updated the PR title and description to reflect what has changed. I think I might also remove the redundant sendgrid.SendGrid API, as it doesn't seem to add much value. The empty request that also gets exposed in the main sendgrid export is also accessible via the sendgrid.SendGrid function, so it seems a bit redundant.

Mind if we just export the main SendGrid function so one can do:

let sendgrid = require('sendgrid');
let sg = sendgrid(process.env.SENDGRID_API_KEY);

Instead of the more verbose:

let sendgrid = require('sendgrid');
let sg = sendgrid.SendGrid(process.env.SENDGRID_API_KEY);

thinkingserious · 2016-07-27T19:22:38Z

Beautiful and thanks again!

adamreisnz · 2016-07-27T19:37:46Z

Ok, done. I've also updated the README.md to reflect the new API.

The emptyRequest method's name with it's new API is a bit misleading. Perhaps we should rename it to something like makeRequest.

jamesdixon · 2016-07-27T23:18:26Z

+1 LGTM

thinkingserious · 2016-07-28T01:53:57Z

@jamesdixon thanks for the vote, I've added your vote to help move this up the queue.

liatreis · 2016-07-29T02:16:49Z

+1

thinkingserious · 2016-08-02T19:41:03Z

@adambuczynski,

I'm now reviewing this pull request :)

So far, I've just tried to run the tests (mocha) and get the following error:

/Users/thinkingserious/Workspace/sendgrid/sendgrid-nodejs/lib/sendgrid.js:78
  this.API = function(request, callback) {
           ^

TypeError: Cannot set property 'API' of undefined
    at SendGrid (/Users/thinkingserious/Workspace/sendgrid/sendgrid-nodejs/lib/sendgrid.js:78:12)
    at Suite.<anonymous> (/Users/thinkingserious/Workspace/sendgrid/sendgrid-nodejs/test/test.js:13:12)
    at context.describe.context.context (/usr/local/lib/node_modules/mocha/lib/interfaces/bdd.js:47:10)
    at Object.<anonymous> (/Users/thinkingserious/Workspace/sendgrid/sendgrid-nodejs/test/test.js:4:1)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Module.require (module.js:367:17)
    at require (internal/module.js:16:19)
    at /usr/local/lib/node_modules/mocha/lib/mocha.js:219:27
    at Array.forEach (native)
    at Mocha.loadFiles (/usr/local/lib/node_modules/mocha/lib/mocha.js:216:14)
    at Mocha.run (/usr/local/lib/node_modules/mocha/lib/mocha.js:468:10)
    at Object.<anonymous> (/usr/local/lib/node_modules/mocha/bin/_mocha:403:18)
    at Module._compile (module.js:413:34)
    at Object.Module._extensions..js (module.js:422:10)
    at Module.load (module.js:357:32)
    at Function.Module._load (module.js:314:12)
    at Function.Module.runMain (module.js:447:10)
    at startup (node.js:142:18)
    at node.js:939:3

adamreisnz · 2016-08-02T19:49:13Z

@thinkingserious interesting, I didn't make any changes to the architectural style of that core function, the this reference was always there:

https://github.com/sendgrid/sendgrid-nodejs/pull/261/files#diff-821a7fec86de51dd671e7622dc4cd6d9R78

Also, I didn't run into this error running the tests.

But I can refactor it to hide the API helper function altogether as a "private" method. Would you prefer that, or do you want to analyze the issue a bit further?

thinkingserious · 2016-08-02T19:50:40Z

@adambuczynski,

How did you run the tests?

adamreisnz · 2016-08-02T19:55:00Z

Via the way you suggested above. However, I tried again and have been able to reproduce it now as well. I think it's related to the way the Sendgrid function is exported now.

I have addressed this by referencing the Sendgrid function directly rather than using this, since the function is not meant to be called as a constructor with new anyway.

Tests are working and passing now.

thinkingserious · 2016-08-02T19:55:26Z

That did the trick :)

adamreisnz · 2016-08-02T19:56:57Z

Excellent, let me know if anything else comes up.

adamreisnz · 2016-08-02T20:09:08Z

Great, thanks for merging. How are you going to version these changes?

thinkingserious · 2016-08-02T20:09:53Z

I'm working on that right now. This will be a major version bump to 4.0. Should be done in about ~10 min.

adamreisnz · 2016-08-02T20:15:26Z

Ok sounds sensible.

Would you like to release it as a RC first and see if we can make other improvements that might be API breaking? That way we could include them without having to do another major release in the future.

thinkingserious · 2016-08-02T20:16:15Z

https://github.com/sendgrid/sendgrid-nodejs/releases/tag/v4.0.0

thinkingserious · 2016-08-02T20:17:26Z

We don't currently have a RC workflow, but that sounds interesting.

For now, we just make major version bumps when something breaks. We follow the semver.org convention.

thinkingserious · 2016-08-02T20:19:11Z

@adambuczynski oops, looks like we forget to remove the let's for var's. We have some breakage on the older node versions: https://travis-ci.org/sendgrid/sendgrid-nodejs/builds/149293172

thinkingserious · 2016-08-02T20:29:59Z

#264

thinkingserious · 2016-08-02T20:31:53Z

For Node v.10 looks like I need to catch this error: https://travis-ci.org/sendgrid/sendgrid-nodejs/jobs/149295924#L228

adamreisnz · 2016-08-02T20:36:59Z

Ok I'll have a look into these issues tomorrow. I am so used to using let that it's become a habit, and I never use var anymore, so they might have snuck in by accident ;)

thinkingserious · 2016-08-02T20:38:00Z

No worries, just about done fixing the issues :)

This will be a 4.0.1 fix.

adamreisnz · 2016-08-02T20:42:49Z

Cool, as for the Promise error you can probably wrap that in an if statement to see if Promise is defined.

thinkingserious · 2016-08-02T20:43:23Z

Yes, the build is passing now :)

https://travis-ci.org/sendgrid/sendgrid-nodejs/builds/149298361

thinkingserious · 2016-08-02T20:50:51Z

https://github.com/sendgrid/sendgrid-nodejs/releases/tag/v4.0.1

Add ESLint configuration and fix issues

1342ecd

thinkingserious added type: community enhancement feature request not on Twilio's roadmap status: cla signed labels Jul 26, 2016

Adam Buczynski added 4 commits July 27, 2016 11:28

Use get empty request helper

d051d8f

New API

7da7ab7

Merge branch 'eslint' into callback-function-signature

18da9c7

Move dep to top, remove duplicate

e41355e

Add helpers

e2b95bd

Doc comments

ea4e9fd

Merge branch 'callback-function-signature' into eslint

bc62953

Remove unneeded eslint plugins

7cc13ef

Update examples and tests with new callback signature

11d6791

Move linting to separate task to avoid breaking tests

9371877

adamreisnz mentioned this pull request Jul 27, 2016

Callback function signature does not follow Node conventions #240

Closed

adamreisnz mentioned this pull request Jul 27, 2016

Library is not promisify-able #206

Closed

adamreisnz changed the title ~~Add ESLint configuration and fix issues~~ Promise API and consistent callback function signature Jul 27, 2016

adamreisnz mentioned this pull request Jul 27, 2016

Why is require('sendgrid-rest') repeated so much in /lib/sendgrid.js? #246

Closed

Remove redundant .SendGrid export, update readme examples

6bef6ce

Update generic API example as well

652210b

thinkingserious mentioned this pull request Jul 28, 2016

Easy to understand example #252

Closed

Avoid this reference

afe0382

thinkingserious merged commit 93763a7 into sendgrid:master Aug 2, 2016

thinkingserious added a commit that referenced this pull request Aug 2, 2016

Version Bump v4.0.0: Pull #261, proper callback function signature

b318ff3

cantremember mentioned this pull request Sep 28, 2016

invoke the API callback upon Error sendgrid/nodejs-http-client#5

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Promise API and consistent callback function signature #261

Promise API and consistent callback function signature #261

adamreisnz commented Jul 26, 2016 •

edited

Loading

adamreisnz commented Jul 26, 2016

thinkingserious commented Jul 26, 2016

adamreisnz commented Jul 26, 2016

thinkingserious commented Jul 27, 2016

adamreisnz commented Jul 27, 2016 •

edited

Loading

thinkingserious commented Jul 27, 2016

adamreisnz commented Jul 27, 2016

adamreisnz commented Jul 27, 2016

thinkingserious commented Jul 27, 2016

adamreisnz commented Jul 27, 2016 •

edited

Loading

thinkingserious commented Jul 27, 2016

adamreisnz commented Jul 27, 2016 •

edited

Loading

jamesdixon commented Jul 27, 2016

thinkingserious commented Jul 28, 2016

liatreis commented Jul 29, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016 •

edited

Loading

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

Promise API and consistent callback function signature #261

Promise API and consistent callback function signature #261

Conversation

adamreisnz commented Jul 26, 2016 • edited Loading

adamreisnz commented Jul 26, 2016

thinkingserious commented Jul 26, 2016

adamreisnz commented Jul 26, 2016

thinkingserious commented Jul 27, 2016

adamreisnz commented Jul 27, 2016 • edited Loading

thinkingserious commented Jul 27, 2016

adamreisnz commented Jul 27, 2016

adamreisnz commented Jul 27, 2016

thinkingserious commented Jul 27, 2016

adamreisnz commented Jul 27, 2016 • edited Loading

thinkingserious commented Jul 27, 2016

adamreisnz commented Jul 27, 2016 • edited Loading

jamesdixon commented Jul 27, 2016

thinkingserious commented Jul 28, 2016

liatreis commented Jul 29, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016 • edited Loading

adamreisnz commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

thinkingserious commented Aug 2, 2016

adamreisnz commented Jul 26, 2016 •

edited

Loading

adamreisnz commented Jul 27, 2016 •

edited

Loading

adamreisnz commented Jul 27, 2016 •

edited

Loading

adamreisnz commented Jul 27, 2016 •

edited

Loading

thinkingserious commented Aug 2, 2016 •

edited

Loading