Callback function signature does not follow Node conventions

[adamreisnz]

This is probably a decision you made to do things a certain way, but it seems with the 3.0.4 version of this library, the callback passed to sg.API does not follow the standard Node callback function signature convention of function (error, data). Instead, the API response is always returned as the first object.

This makes it hard to determine if something went wrong, as we are now responsible for deciphering the raw API response and analyse the response headers and status codes manually.

I don't feel that this should be the responsibility of user land, but of the library itself. What if status codes change? What if the response format changes? What if an error occurs and the returned data format becomes different, or null or undefined?

The very least that can be done to address this is for the library to determine if an error occurred, and if yes, populate the first parameter with an error object (it can have the response attached to it). If no error occurs, return null as the first parameter and the response object as the second parameter.

[adamreisnz]

My current solution is to check the status code for success, e.g.:

function sendMail(mail) {
  return new Promise((resolve, reject) => {

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

    //Send request
    sg.API(request, response => {
      if (response && response.statusCode &&
        response.statusCode >= 200 && response.statusCode <= 299) {
        resolve(response);
      }
      reject(new SendMailError(
        'Sendgrid response error ' + response.statusCode
      ));
    });
  });
}

As you can see this is quite verbose though and requires a lot of boilerplate to simply send an email and check if it worked or not.

Proposed API would be something like:

function sendMail(mail) {
  return new Promise((resolve, reject) => {

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

    //Send request
    sg.API(request, (error, response) => {
      if (error {
        reject(new SendMailError(error);
      }
      resolve(response);
    });
  });
}

Or better yet, with promises returned:

function sendMail(mail) {

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

  //Send request
  return sg.API(request);
}

And, ideally, we'd see the return of a dedicated mail sending helper which would create the proper request for us in the background, eliminating the need for a sendMail helper and boilerplate altogether:

sg.sendMail(mail);

If this is something you are interested in pursuing I will look into making some PR's for it.

[thinkingserious]

Thanks @adambuczynski,

Generally, we are planning to update the error handling, but we have not scoped that out yet. We will take this feedback into consideration when that time comes.

If you want to provide a pull request, we need a signed CLA: https://github.com/sendgrid/sendgrid-nodejs/blob/master/CONTRIBUTING.md#cla

We have setup a helper structure for the kind of enhancements you propose: https://github.com/sendgrid/sendgrid-nodejs/tree/master/lib/helpers/mail. You can either help modify that one, or create your own in the helpers directory.

Thanks for your support!

[adamreisnz]

Yes I have seen the helper for mail. Do you want to create a mail sending helper on that object?

[thinkingserious]

That sounds good to me :)

[beldenge]

The suggestions @adambuczynski made are spot on. I just updated to the v3 API and was very disappointed that I can't use a simple helper function anymore and instead have to write a bunch of boiler plate code. I can no longer even wrap it in a Bluebird promise without writing ugly error handling code.

[thinkingserious]

Awesome, thanks for the validation @beldenge!

Please follow this issue for progress. If @adambuczynski creates a pull request, I'll post it here; otherwise, we'll add this to our roadmap for one of the next library updates.

[adamreisnz]

@thinkingserious I've just mailed you the signed CA, I hope to have some time next week to look into this.

[thinkingserious]

@adambuczynski we have received it and thanks for the feedback!

[adamreisnz]

Hi guys, haven't forgotten about this, but I've been a bit busy with several other projects. Still hope to look into this at some stage :)

[adamreisnz]

@thinkingserious I've had a look at your source code, but it looks a little bit messy. If I make edits, my code linter will make a lot of automated changes to it (e.g. add missing semi colons, use consistent quote style for strings, use consistent spacings between language elements, etc. etc)

Are you ok with that happening? I don't think I will be able to work with the code otherwise, as it hurts my brain to look at it :)

PS why don't you use linters :)

[thinkingserious]

@adambuczynski,

We use this linter: http://eslint.org along with the standard style guide.

Which one do you use? If we can sync up and use the same linter that would be wonderful.

[adamreisnz]

I use ESLiint as well, but it doesn't seem like you apply a lot of rules as there are still so many inconsistencies in the code.

This is the config that I use: https://gist.github.com/adambuczynski/1fa24bcfc5d17b8d26e4c39ffca7560e#file-eslintrc-node-yaml

I think it provides a good balance between consistency and best practices, yet without being too annoying.

[adamreisnz]

There is no .eslintrc.yaml config file in your project. Could you add/create one with your preferred rules so that I can use it as well?

[thinkingserious]

@adambuczynski,

I'm happy to use yours, it looks solid :)

Please add it as part of your commit.

[adamreisnz]

@thinkingserious it is setup for ES6 features though, e.g. warning when you use var instead of let and using the ES6 parser.

Is that ok, or do you prefer to use ES5 only for backwards compatibility?

[thinkingserious]

We need to support Node.js versions: "0.10", "0.12", "iojs", "4"

I'm fine with the more modern approach if we don't break support for those versions.

[adamreisnz]

Ok, I've kept the var's and just fixed code style issues that came up. Created a PR for that separately from this issue.

[adamreisnz]

@thinkingserious I am also implementing a Promise API, but native promises were only introduced in Node 0.11.13.

Several solutions, let me know which (or combination) you prefer:

• Use bluebird promises as a dependency
• Check if promises are supported or not, throw an error if they aren't but if people try to use them
• Allow users to set the promise implementation they want to use by simply setting sendgrid.Promise to whatever value.

Probably a combination of 2 and 3 would be simplest and most flexible.

[thinkingserious]

Agreed.

I like this option: "Check if promises are supported or not, throw an error if they aren't but if people try to use them", but also allowing users to specify the implementation they would like.

[adamreisnz]

@thinkingserious perfect. I have something ready for review, do you want me to push it to the same PR, or create a separate PR for it?

[thinkingserious]

@adambuczynski you can keep in the same PR, thanks!

[adamreisnz]

See #261

[adamreisnz]

@beldenge would you mind adding a +1 to the PR #261 so the Sendgrid team can move it along and consider it for merge? Thanks!

[thinkingserious]

@adambuczynski I'll add the +1 now :)

[thinkingserious]

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