nylas / nylas-nodejs

A NodeJS wrapper for the Nylas REST API for email, contacts, and calendar.
MIT License
167 stars 117 forks source link

undefined `replyToMessageId` results in empty string #484

Closed impowski closed 9 months ago

impowski commented 1 year ago

Describe the bug A clear and concise description of what the bug is.

If you set replyToMessageId to undefined it results in an empty string inside of the Draft model thus creates an error from the API call Bad Request: Invalid 'message_id'

To Reproduce Some steps involved to reproduce the bug and any code samples you can share.

const draft = new Draft(nylas, {
    to,
    subject,
    body,
    replyToMessageId: undefined,
  })

Expected behavior A clear and concise description of what you expected to happen.

Should be not present or undefined inside of the Draft object

SDK Version: ^6.10.0 Providing the SDK version can help with the reproduction of the issue and to know if a change could have broken something.

Additional context Add any other context about the problem here.

relaxedtomato commented 12 months ago

@impowski - let me take a look and circle back.

To clarify, the issue is an undefined replyToMessageId causes the API to fail?

Is the idea to specify replyToMessageId if it exists? Like would like something like the following work:

const draft = new Draft(nylas, {
    to,
    subject,
    body,
    ...(messageId ? { replyToMessageId: messageId } : { }),
  })
impowski commented 11 months ago

@relaxedtomato I just create a completely different Draft object if I have a replyToMessageId present with it.

relaxedtomato commented 11 months ago

Okay, so just to summarize, an improvement to the SDK is that replyToMessageId should not be included if specified as undefined as this causes an api error.

mrashed-dev commented 11 months ago

Hey @impowski sorry just getting caught up on this. Why are you setting replyToMessageId to undefined? It's optional and thus undefined by default: https://github.com/nylas/nylas-nodejs/blob/c4a54f56d15200dbc0d1c87d8cbe8bc812273054/src/models/draft.ts#L13

If you are trying to update an existing draft and are trying to "nullify"/remove the reply_to_message_id field which was already set, that is currently not possible on the Nylas API. It's a limitation beyond the SDK's control.

If I misunderstood your question please let me know, thanks!

impowski commented 11 months ago

Hey @impowski sorry just getting caught up on this. Why are you setting replyToMessageId to undefined? It's optional and thus undefined by default: https://github.com/nylas/nylas-nodejs/blob/c4a54f56d15200dbc0d1c87d8cbe8bc812273054/src/models/draft.ts#L13

If you are trying to update an existing draft and are trying to "nullify"/remove the reply_to_message_id field which was already set, that is currently not possible on the Nylas API. It's a limitation beyond the SDK's control.

If I misunderstood your question please let me know, thanks!

I have a wrapper function which either sends a new message or replies to a message if it's present.

The issue is that API transformer or something includes it inside of the request when it should not be like any other parameter.

I'm not trying to do anything else I'm just expecting if it's undefined it is not included into request that's all

mrashed-dev commented 11 months ago

@impowski Is it possible for you to share the wrapper function? A little more context can maybe help me debug this as I'm not able to reproduce this on my end. If you do not feel comfortable sharing source code here you can open a ticket via support@nylas.com and we can have a private conversation over there. Thanks!

impowski commented 11 months ago

@impowski Is it possible for you to share the wrapper function? A little more context can maybe help me debug this as I'm not able to reproduce this on my end. If you do not feel comfortable sharing source code here you can open a ticket via support@nylas.com and we can have a private conversation over there. Thanks!

@mrashed-dev

interface SendEmailParameters {
  to: string | EmailParticipantProperties[]
  subject: string
  body: string
  replyToMessageId?: string
}

export async function sendEmail(params: SendEmailParameters) {
  const { to, subject, body, replyToMessageId } = params

  const draft = new Draft(nylas, {
        to: typeof to === 'string' ? [{ email: to }] : to,
        subject,
        body,
        replyToMessageId,
      })

  const sentDraft = await draft.send()

  return await getMessageById(sentDraft.id!)
}
mrashed-dev commented 9 months ago

Thanks @impowski it seems like the SDK defaults the value to an empty string, which the API does not like. Now we omit that field if it's set to an empty string. Fix will be included in the next release.