sendgrid-nodejs icon indicating copy to clipboard operation
sendgrid-nodejs copied to clipboard

MailDataRequired requires MailContent?

Open 0x80 opened this issue 5 years ago • 26 comments

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 avatar Mar 07 '20 10:03 0x80

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

0x80 avatar Mar 07 '20 11:03 0x80

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

jonbcampos-alto avatar Mar 08 '20 00:03 jonbcampos-alto

Considering this a duplicate of https://github.com/sendgrid/sendgrid-nodejs/issues/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.

childish-sambino avatar Mar 09 '20 15:03 childish-sambino

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

rodrigomf24 avatar Mar 11 '20 02:03 rodrigomf24

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

@rodrigomf24 Does mailData not have one of those?

childish-sambino avatar Mar 11 '20 14:03 childish-sambino

@childish-sambino no just a templateId and dynamicTemplateData

rodrigomf24 avatar Mar 11 '20 15:03 rodrigomf24

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

childish-sambino avatar Mar 11 '20 15:03 childish-sambino

Is this fix released?

saveraa avatar Apr 05 '20 20:04 saveraa

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 avatar Apr 05 '20 20:04 saveraa

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.

saveraa avatar Apr 05 '20 20:04 saveraa

Yes, this was released in 6.5.5

childish-sambino avatar Apr 06 '20 14:04 childish-sambino

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

isaachinman avatar Apr 06 '20 15:04 isaachinman

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

childish-sambino avatar Apr 06 '20 15:04 childish-sambino

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

isaachinman avatar Apr 06 '20 15:04 isaachinman

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 };

childish-sambino avatar Apr 06 '20 15:04 childish-sambino

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

isaachinman avatar Apr 06 '20 15:04 isaachinman

Then just , MailDataRequired: MailDataRequired };?

childish-sambino avatar Apr 06 '20 16:04 childish-sambino

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 }

isaachinman avatar Apr 06 '20 16:04 isaachinman

@isaachinman Think this (hack) should fix it: https://github.com/sendgrid/sendgrid-nodejs/pull/1102

childish-sambino avatar Apr 23 '20 18:04 childish-sambino

still having the same issue, and proper workaround?

yenicelik avatar Jun 07 '21 10:06 yenicelik

Still same issue with version 7.6.0

mkmrcodes avatar Jan 20 '22 22:01 mkmrcodes

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);

peterj avatar Feb 07 '22 00:02 peterj

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

HubbardJacob avatar Feb 08 '22 04:02 HubbardJacob

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

alzayatn avatar Feb 23 '22 19:02 alzayatn

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 avatar Feb 25 '22 16:02 childish-sambino

Hello, any workaround for this ? thanks !

mgara avatar Apr 20 '22 03:04 mgara