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

MailDataRequired requires MailContent? #1057

Open
0x80 opened this issue Mar 7, 2020 · 26 comments
Open

MailDataRequired requires MailContent? #1057

0x80 opened this issue Mar 7, 2020 · 26 comments
Labels
status: help wanted requesting help from the community type: bug bug in the library

Comments

@0x80
Copy link

0x80 commented Mar 7, 2020

Issue Summary

I'm running into a problem with Typescript because it doesn't let me build an email without a content field. Is the MailDataRequired correct?

I never used to send content with my emails. Only substitutions. The content / templates live on your servers.

Code Snippet

export type MailDataRequired = MailData & (
    { text: string } | { html: string } | { templateId: string } | { content: MailContent[] & { 0: MailContent } });

Exception/Log

# paste exception/log here

Technical details:

  • sendgrid-nodejs version: @sendgrid/client@^6.5.3
@0x80
Copy link
Author

0x80 commented Mar 7, 2020

BTW: 6.5.0 doesn't seem to have this weird { 0: MailContent } requirement, but still content is required.

@jonbcampos-alto
Copy link

watching this one too. updated my package and now not sure about this

@childish-sambino
Copy link
Contributor

Considering this a duplicate of #1056, unless I'm mistaken.

#1041 introduced in the 6.5.4 release required one of text, html, or content. It was later pointed out that templateId by itself should also be allowed. This was fixed as part #1053 but has yet to be released.

@rodrigomf24
Copy link

@childish-sambino this is still an issue on 6.5.4 the type is defined like this:

type MailDataRequired = MailData & (
  { text: string } | { html: string } | { content: MailContent[] & { 0: MailContent } });

That is making content a required property, is there a reason for that when MailData defines it as a conditional property? I fixed it by passing:

{...mailData, content: undefined}

@childish-sambino
Copy link
Contributor

#1041 introduced in the 6.5.4 release required one of text, html, or content.

@rodrigomf24 Does mailData not have one of those?

@rodrigomf24
Copy link

@childish-sambino no just a templateId and dynamicTemplateData

@childish-sambino
Copy link
Contributor

@rodrigomf24

It was later pointed out that templateId by itself should also be allowed. This was fixed as part #1053 but has yet to be released.

@saveraa
Copy link

saveraa commented Apr 5, 2020

Is this fix released?

@saveraa
Copy link

saveraa commented Apr 5, 2020

I just got version 7.0.0 and I still see this error

TSError: ⨯ Unable to compile TypeScript: src/email-templates/verification-email-template.ts(15,25): error TS2769: No overload matches this call. Overload 1 of 2, '(data: MailDataRequired, isMultiple?: boolean | undefined, cb?: ((err: Error | ResponseError, result: [ClientResponse, {}]) => void) | undefined): Promise<...>', gave the following error.

@saveraa
Copy link

saveraa commented Apr 5, 2020

If this has changed in 7.0.0, where is latest documentation for the send api please? Can't find much here. @childish-sambino

Thanks.

@childish-sambino
Copy link
Contributor

Yes, this was released in 6.5.5

@isaachinman
Copy link

@childish-sambino What do you think about exporting MailDataRequired from the @sendgrid/mail package as well?

@childish-sambino
Copy link
Contributor

@isaachinman I've no issue with that. If you want to submit a PR I can review it.

@isaachinman
Copy link

@childish-sambino I had a look around, and am unfamiliar with the module approach of export = being taken here. I'm not clear on how this can be expanded without introducing breaking changes.

Happy to help if you can point me in the right direction.

@childish-sambino
Copy link
Contributor

Think this line can just be updated: https://github.com/sendgrid/sendgrid-nodejs/blob/master/packages/mail/src/mail.d.ts#L37

Like this maybe:

declare const mail: MailService & { MailService: typeof MailService, MailDataRequired: typeof MailDataRequired };

@isaachinman
Copy link

No, that won't work as MailDataRequired is already a type. The use of declare const makes things slightly complicated.

@childish-sambino
Copy link
Contributor

Then just , MailDataRequired: MailDataRequired };?

@isaachinman
Copy link

No, what I am saying is that literally adds a MailDataRequired attribute to the default export. If you tried out your latest suggestion, you'd get a refers to a value, but is being used as a type error.

I haven't seen any other packages managing types with this sort of module approach, so I can't really offer you any suggestions. You need a way of supporting named exports.

Normally I'd expect to see something like this:

export default mail
export { MailDataRequired }

@childish-sambino
Copy link
Contributor

@isaachinman Think this (hack) should fix it: #1102

@yenicelik
Copy link

still having the same issue, and proper workaround?

@mkmrcodes
Copy link

Still same issue with version 7.6.0

@peterj
Copy link

peterj commented Feb 7, 2022

Not perfect, but it allows you to go past the error:

// @ts-expect-error: https://github.com/sendgrid/sendgrid-nodejs/issues/1057
await sgMail.sendMultiple(msg);

@HubbardJacob
Copy link

Was also having this issue with 7.6.0. Had to force compiler to ignore it.

@alzayatn
Copy link

alzayatn commented Feb 23, 2022

Also having the same issue with version 7.6.1

For reference I was digging around and I see:

send(data: MailDataRequired | MailDataRequired[], isMultiple?: boolean, cb?: (err: Error | ResponseError, result: [ClientResponse, {}]) => void): Promise<[ClientResponse, {}]>;

Where MailDataRequired is:

export type MailDataRequired = MailData & (
    { text: string } | { html: string } | { templateId: string } | { content: MailContent[] & { 0: MailContent } });

but conflicts with MailData where content is optional and the types mismatch:


export interface MailData {
  ...
  content?: MailContent[],
  ...
}

Can this be resolved? I'd rather not have to ignore it...

@childish-sambino
Copy link
Contributor

This issue has been added to our internal backlog to be prioritized. Pull requests and +1s on the issue summary will help it move up the backlog.

@childish-sambino childish-sambino removed the status: duplicate duplicate issue label Feb 25, 2022
@childish-sambino childish-sambino added status: help wanted requesting help from the community type: bug bug in the library labels Feb 25, 2022
@mgara
Copy link

mgara commented Apr 20, 2022

Hello, any workaround for this ? thanks !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: help wanted requesting help from the community type: bug bug in the library
Projects
None yet
Development

No branches or pull requests