MarcosNicolau / whatsapp-business-sdk

Node.js connector for the WhatsApp Business APIs with TypeScript support, integration tests and more.
MIT License
60 stars 18 forks source link

Some properties should be optional in type declarations #17

Closed rnbrady closed 8 months ago

rnbrady commented 8 months ago

Hi! πŸ‘‹

Firstly, thanks for your work on this project! πŸ™‚

Today I used patch-package to patch whatsapp-business@1.7.1 for the project I'm working on.

Here is the diff that solved my problem:

diff --git a/node_modules/whatsapp-business/dist/src/types/messages.d.ts b/node_modules/whatsapp-business/dist/src/types/messages.d.ts
index 8fc6383..5f9697a 100644
--- a/node_modules/whatsapp-business/dist/src/types/messages.d.ts
+++ b/node_modules/whatsapp-business/dist/src/types/messages.d.ts
@@ -399,7 +399,7 @@ export type TemplateMessageLanguage = {
     /**
      * The language policy the message should follow. See Language Policy Options for more information.
      */
-    policy: "deterministic";
+    policy?: "deterministic";
     /**
      * The code of the language or locale to use. Accepts both language and language_locale formats (e.g., en and en_US).
      */
@@ -408,8 +408,8 @@ export type TemplateMessageLanguage = {
 export type TemplateMessage = {
     name: string;
     language: TemplateMessageLanguage;
-    category: LiteralUnion<"AUTHENTICATION" | "MARKETING " | "UTILITY">;
-    components: TemplateMessageComponent[];
+    category?: LiteralUnion<"AUTHENTICATION" | "MARKETING " | "UTILITY">;
+    components?: TemplateMessageComponent[];
 };
 export type TextMessage = {
     /**
diff --git a/node_modules/whatsapp-business/dist/src/types/webhooks.d.ts b/node_modules/whatsapp-business/dist/src/types/webhooks.d.ts
index c3cc084..a4f4ff8 100644
--- a/node_modules/whatsapp-business/dist/src/types/webhooks.d.ts
+++ b/node_modules/whatsapp-business/dist/src/types/webhooks.d.ts
@@ -312,7 +312,7 @@ export type WebhookStatus = {
      * The WhatsApp ID for the customer that the business, that is subscribed to the webhooks, sent to the customer
      */
     recipient_id: string;
-    status: "delivered" | "read" | "sent";
+    status: "delivered" | "failed" | "read" | "sent";
     /**
      * Date for the status message in unix
      */

This issue body was partially generated by patch-package.

rnbrady commented 8 months ago

PS: Just seen you already merged my previous PR, thanks! Let me know here if you'd like a PR for the other fields above.

MarcosNicolau commented 8 months ago

Hello!

I'm glad to hear that you are finding this project useful, and thanks for contributing!

Regarding the issue, I have just checked and you are absolutely correct. If you don't mind, please send the pull request, and I'll accept it. πŸ‘

MarcosNicolau commented 8 months ago

Hi @rnbrady! Do you still want to send the pr? If not, I'll do it np :+1: .

rnbrady commented 8 months ago

I'm not so confident in the changes any more.

  1. The diff above makes language.policy optional, because it's missing in the example that I used, and the API accepted requests that I made without it. However, the docs state that it's required, so it should probably stay that way in your lib.
Screenshot 2023-10-26 at 16 00 28
  1. Then category seems to be something that you would specify when creating a template but not when using it. When creating a template it would be required but when using a template it would not be present. Do the two scenarios share the same type? I would expect a TemplateMessage to use a MessageTemplate and only the latter would have a category.

I submitted #18 in case it's useful.

Side note: I'm using your lib with Next.js, so I'm import the handler like this:

import { Webhook, WebhookEvents } from 'whatsapp-business';
import { webhookHandler } from 'whatsapp-business/dist/src/webhooks/helpers';

I might send a PR in the future to simplify that.

MarcosNicolau commented 8 months ago

Good morning @!

First of all, thank you very much for this careful response!

About the policy, let's better leave it required, because now that you mention it, I remember that a few months ago, the API failed when I didn't specify it.

In regards to the category, I've seen this on the changelog

image

Which affects all the APIs. Also, when I implemented it, I tested what happened if I didn't provide the category and it failed. But I was also confused with the fact that in the actual docs it was mentioned.

Anyway, I will accept your pr because it makes sense to leave it optional as it is in the docs, but maybe we could add some doc comment that warns that if the API fails then try by specifying the category.

Again, thank you some much for the pr, and about the import; anything that improves the code is welcomed!!

github-actions[bot] commented 8 months ago

:tada: This issue has been resolved in version 1.7.2 :tada:

The release is available on:

Your semantic-release bot :package::rocket: