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

fix: Changed contentId to content_id for correct type #1364

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

YoussefHenna
Copy link

@YoussefHenna YoussefHenna commented May 28, 2022

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

  • I acknowledge that all my contributions will be made under the project's license
  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the Contribution Guidelines and my PR follows them
  • I have titled the PR appropriately
  • I have updated my branch with the main branch
  • I have added tests that prove my fix is effective or that my feature works
  • I have added the necessary documentation about the functionality in the appropriate .md file
  • I have added inline documentation to the code I modified

If you have questions, please file a support ticket.

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
@itseramin
Copy link

Please merge.

@depyronick
Copy link

omg, really...

@depyronick
Copy link

depyronick commented Nov 25, 2022

if anyone needs a hand...

import { AttachmentData } from "@sendgrid/helpers/classes/attachment";

attachments: <(AttachmentData & { content_id: string })[]>[{
      disposition: 'inline',
      filename: 'image.jpg',
      content: base64Content,
      content_id: "inlineimage",
      type: 'image/jpeg'
}]

@davidchalifoux
Copy link

Wow I just spent a good chunk of time trying to figure out why SendGrid wasn't setting the Content-ID header. This really should be merged ASAP @twilio-dx

@tianhuil
Copy link

@SendGridDX: can we merge this. It's insane that the field is misnamed in the official node package.

Copy link

@depyronick depyronick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

Copy link

@Codeprinz-1 Codeprinz-1 left a 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants