-
Notifications
You must be signed in to change notification settings - Fork 778
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
fix: Changed contentId to content_id for correct type #1364
base: main
Are you sure you want to change the base?
Conversation
In order for the 'Content-ID' header to be set for the attachment, the parameter 'content_id' has to be set. 'contentId' isn't registered as the header, and the header is not provided in the email. The only workaround is to not use types, which defeats the purpose. This should fix the type
Please merge. |
omg, really... |
if anyone needs a hand...
|
Wow I just spent a good chunk of time trying to figure out why SendGrid wasn't setting the |
@SendGridDX: can we merge this. It's insane that the field is misnamed in the official node package. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me. @sendgrid-argo-cd @sendgrid-ci @sendgrid-github-readonly @sendgrid-jira @SendGridDX can we merge this type fix or update the code to use contentId
. It doesn't make sense for the code to expect content_id
and the type expects contentId
. This is the official node package, I spent a bit of time trying to figure out what was wrong with my code only to find out hat it was this all along.
Fixes
In order for the 'Content-ID' header to be set for the attachment, the parameter 'content_id' has to be set. 'contentId' isn't registered as the header, and the header is not provided in the email. The only workaround is to not use types, which defeats the purpose.
This should fix the type
Checklist
If you have questions, please file a support ticket.