mollie / mollie-api-node

Official Mollie API client for Node
http://www.mollie.com
BSD 3-Clause "New" or "Revised" License
228 stars 62 forks source link

Questions and remarks regarding some types #303

Closed oga-sven closed 1 year ago

oga-sven commented 1 year ago

Hi there. I have a few question remarks. I apologize in advance if some of these remarks are "obviously" wrong. If that is the case I hope to learn

I've been setting up new handlers for Mollie using the types from the library rather than my custom types. I noticed a few things:

On type OrderAddress, Phone seems to be optional. However I believe the mollie API requires a phone number to be present. Is there a reason this property is optional?

the metadata property is of type any. Would it be possible to use a generic? example:

type TMetaData = {
   order_notes: string;
   discount_code?: string
}

const order = mollieClient.orders.get<TMetaData>("ID") //optional generic. If you dont need metadata you dont need to use it
const orderMetaData = order.metadata // returns object of type TMetaData

Finally, I noticed a lot of the typenames are bland. With order being just "Order" for example. This might cause issues and confusion with variables using the same name. It would perhaps be better using something like TOrder

Sorry if some things don't make sense. English is not my main language.

Pimm commented 1 year ago

Hey, Sven!

On type OrderAddress, Phone seems to be optional. However I believe the mollie API requires a phone number to be present. Is there a reason this property is optional?

According to docs.mollie.com: "[s]ome payment methods require this information." This is why it's marked as optional in the type definition. But yes, indeed, you will have to provide a phone number sometimes.

the metadata property is of type any. Would it be possible to use a generic?

That is definitely possible, but I don't think it would be useful. You can write this to get the same effect as your snippet:

const order = await mollieClient.orders.get("ID")
const orderMetaData: TMetaData = order.metadata

Finally, I noticed a lot of the typenames are bland. With order being just "Order" for example. This might cause issues and confusion with variables using the same name. It would perhaps be better using something like TOrder

In some languages, it is common to prefix names like that. I don't see many people do that in TypeScript, however. Variables in JavaScript/TypeScript are conventionally in dromedaryCase 🐪, I think that makes it unlikely that someone will confuse Order for a variable. Additionally, the popular IDEs will render variable names and type names in distinct colours.

oga-sven commented 1 year ago

Thanks for the response and feedback!