Skip to content

fix: Changed contentId to content_id for correct type#1364

Open
YoussefHenna wants to merge 2 commits intosendgrid:mainfrom
YoussefHenna:patch-1
Open

fix: Changed contentId to content_id for correct type#1364
YoussefHenna wants to merge 2 commits intosendgrid:mainfrom
YoussefHenna:patch-1

Conversation

@YoussefHenna
Copy link
Copy Markdown

@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
Copy Markdown

Please merge.

@depyronick
Copy link
Copy Markdown

omg, really...

@depyronick
Copy link
Copy Markdown

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
Copy Markdown

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
Copy Markdown

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

Copy link
Copy Markdown

@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
Copy Markdown

@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.

6 participants