-
Notifications
You must be signed in to change notification settings - Fork 780
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(client): Fix typescript issue in the Attachment model #1402
base: main
Are you sure you want to change the base?
fix(client): Fix typescript issue in the Attachment model #1402
Conversation
I've just hit this issue... what needs to happen to get this PR reivewed? |
I do not know. @thinkingserious can yo suggest? |
Review in progress. |
@@ -16,7 +16,7 @@ const msg = { | |||
filename: 'some-attachment.txt', | |||
type: 'plain/text', | |||
disposition: 'attachment', | |||
content_id: 'mytext' | |||
contentId: 'mytext' |
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.
As per sendgrid open api specification, it is content_id
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.
ThecontentId
property is used internally in the Attachment class. You are correct that the API expects the content_id
property, and that is exactly what we send. The toJSON method of the Mail class handles this conversion, transforming the internal camelCase structure into the snake_case format expected by the API.
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.
@sbansla could you take a look, please?
Fixes
Motivation
This PR addresses an inconsistency in the TypeScript type definition for the Attachment model that prevents proper use of TypeScript with the library.
Issue
The Attachment model's TypeScript type definition does not match the expected data structure for the SendGrid API. Specifically, the
contentId
field should becontent_id
to align with the API requirements. This discrepancy does not impact JavaScript users but hinders TypeScript usage by causing type errors.Solution
I store attachment details in the appropriate class with the
toJSON
method to ensure the content_id field is correctly included in the API request payload.Also, I added tests to confirm the content_id field is correctly handled and included in the API request payload.
Checklist