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

Update documentation to reflect use of promises. #870

Closed
Berkmann18 opened this issue Jan 12, 2019 · 6 comments · Fixed by #877
Closed

Update documentation to reflect use of promises. #870

Berkmann18 opened this issue Jan 12, 2019 · 6 comments · Fixed by #877
Labels
difficulty: medium fix is medium in difficulty status: help wanted requesting help from the community type: docs update documentation change not affecting the code

Comments

@Berkmann18
Copy link
Contributor

It's not an issue per se but more of a README/documentation suggestion.
That concerns the example(s) on the README/USE_CASES.

The basic example is this:

const sgMail = require('@sendgrid/mail');
sgMail.setApiKey(process.env.SENDGRID_API_KEY);
const msg = {
  to: '[email protected]',
  from: '[email protected]',
  subject: 'Sending with SendGrid is Fun',
  text: 'and easy to do anywhere, even with Node.js',
};
sgMail.send(msg);

But reading this makes me think it's a synchronous code which is fine but I didn't realise it was returning a Promise until running it live on Node v11.

I'm aware of the callback version:

sgMail
  .send(msg, (error, result) => {
    if (error) {
      //Do something with the error
    }
    else {
      //Celebrate
    }
  });

And that there's thenable use cases here and here that uses:

//...
sgMail
  .send(msg)
  .then(() => console.log('Mail sent successfully'))
  .catch(error => console.error(error.toString()));

But if someone runs an async code without rejection handling (so not using either try { await ... } catch (err) { ... } in ES8 or Promise.catch(...) in ES6) it can be noisy and possibly leave room for bugs and errors if someone were to use the package without a proper error handling mechanism.

So for example having this:

  • ES6
const sgMail = require('@sendgrid/mail');
sgMail.setApiKey(process.env.SENDGRID_API_KEY);
const msg = {
  to: '[email protected]',
  from: '[email protected]',
  subject: 'Sending with SendGrid is Fun',
  text: 'and easy to do anywhere, even with Node.js',
};
sgMail
  .send(msg)
  .then(() => {}, console.error); //or .then(() => {}).catch(console.error)
  • ES8
const sgMail = require('@sendgrid/mail');
sgMail.setApiKey(process.env.SENDGRID_API_KEY);
const msg = {
  to: '[email protected]',
  from: '[email protected]',
  subject: 'Sending with SendGrid is Fun',
  text: 'and easy to do anywhere, even with Node.js',
};

(async () => {
  try {
    await sgMail.send(msg);
  } catch (err) {
    console.error(err.toString());
  }
})();

Technical details:

@thinkingserious thinkingserious added status: help wanted requesting help from the community difficulty: medium fix is medium in difficulty up-for-grabs type: docs update documentation change not affecting the code labels Jan 17, 2019
@thinkingserious
Copy link
Contributor

Great idea @Berkmann18! Thanks for taking the time to make this suggestion.

I have labeled it appropriately to get this change integrated into our docs.

@thinkingserious thinkingserious changed the title @sendgrid/mail handling Update documentation to reflect use of promises. Jan 17, 2019
@Berkmann18
Copy link
Contributor Author

@thinkingserious You're welcome.

Berkmann18 added a commit to Berkmann18/sendgrid-nodejs that referenced this issue Jan 18, 2019
I included the missing bits to handle the asynchronous `send()` call
including both ES6 and ES8 approaches which closes sendgrid#870.
childish-sambino pushed a commit that referenced this issue Feb 13, 2020
)

I included the missing bits to handle the asynchronous `send()` call
including both ES6 and ES8 approaches which closes #870.
@Sammy-White
Copy link

Thanks man this really helped me to fix an issue with my code.

Thanks a lot.

@Berkmann18
Copy link
Contributor Author

@Sammy-White No problem, I'm glad I helped.

@ogrotten
Copy link

ogrotten commented Jul 1, 2020

So I was digging around to see if this was promise-able or chainable. Nothing says it is, or even hints to it . . . and then I tripped across this issue.

It's quite a missed boat.

@Berkmann18 your doc suggestion is great. Thought about making a PR?

@Berkmann18
Copy link
Contributor Author

@ogrotten Yup, see #877.

@sendgrid sendgrid locked as resolved and limited conversation to collaborators Jul 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
difficulty: medium fix is medium in difficulty status: help wanted requesting help from the community type: docs update documentation change not affecting the code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants