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

TypeScript types allow mails with missing bodies #978

Closed
garybernhardt opened this issue Sep 4, 2019 · 4 comments · Fixed by #1041
Closed

TypeScript types allow mails with missing bodies #978

garybernhardt opened this issue Sep 4, 2019 · 4 comments · Fixed by #1041
Labels
difficulty: easy fix is easy in difficulty status: work in progress Twilio or the community is in the process of implementing type: bug bug in the library

Comments

@garybernhardt
Copy link

Issue Summary

Each SendGrid mail requires either a text key or an html key. If neither is present, the server will reject the mail.

The SendGrid TypeScript types' MailData type is the type passed to sendgrid.send. It contain this:

export interface MailData {
  /* other keys elided */
  text?: string,
  html?: string,
  /* other keys elided */
}

This is wrong, because it means that both text and html are allowed to be absent. If you send such a request to the server, it will always be rejected with HTTP 400 Bad Request.

Steps to Reproduce

  1. Install the sendgrid/mail NPM package.
  2. Write TypeScript code that calls sendgrid.send({from, to, subject}) (with no text or html provided).
  3. That code type checks.
  4. Run that code with a valid SendGrid API key.
  5. The server will always reject it because it has no body.

This is a bug in the types: they allow me to do something that's (1) invalid, and (2) statically preventable in TypeScript's type system. Our user feedback form was down in production for 24 hours because of this bug.

Technical details:

This can be fixed by doing something like:

export interface MailData {
  /* other keys elided */
} & (
  {text: string} | {html: string} | {text: string, html: string}
)

With that change, the user can no longer call sendgrid.send without providing at least one type of body. (There are probably more types of bodies that I don't know about, and which would need to be considered in the union as well. I assume that's what the content key is, for example. But I only know a relatively small part of SendGrid's API.)

@childish-sambino
Copy link
Contributor

TS doesn't like merging a definition with an interface so it either needs to be a new type or switched to a type. I'm leaning towards making a new type that is used specifically by the send* methods since not having a text or html value is only applicable during the send. For instance, we could later change the send* methods to also accept an instance of Mail (which the JS code actually supports) and you can build a Mail instance from a partially complete MailData object and then fill in the other bits. For example:

const msg = new Mail({
    subject: "Subjective"
});
msg.addTextContent("body");
send(msg);

This works fine in JS and could be made to work in TS. But anyway, I'll work on a PR.

@childish-sambino childish-sambino added difficulty: easy fix is easy in difficulty status: work in progress Twilio or the community is in the process of implementing type: bug bug in the library labels Feb 14, 2020
@garybernhardt
Copy link
Author

Thanks for fixing this, @childish-sambino!

@mconatser
Copy link

I think this fix is causing typescript errors when sending transactional templates which from the documentation don't seem to require a text or html key: https://github.com/sendgrid/sendgrid-nodejs/blob/master/use-cases/transactional-templates.md. See issue below

image

@childish-sambino
Copy link
Contributor

@mconatser See #1057

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
difficulty: easy fix is easy in difficulty status: work in progress Twilio or the community is in the process of implementing type: bug bug in the library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants