mollie / mollie-api-node

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

Use type instead of enum #284

Open shwao opened 2 years ago

shwao commented 2 years ago

Some things that could be a TypeScript type are enum which is quite developer unfriendly. Code completion and many checks just don't work.

The Problem

For example if you have a Payment and want to check its status, the IDE (VS Code in my case) can't give you anything to autocomplete. The type check works though.

Here you should have the statuses as suggestions: Screenshot 2022-07-21 at 14 48 58

This is the case for every enum including Locale, PaymentMethod etc.

While the missing suggestions are basically just an inconvenience (and missing out on fully utilizing TypeScript and the IDE) using the embed option of the mollieClient.payments.get method just does not work without using any because of the enum:

Screenshot 2022-07-21 at 14 51 51

In contrast to the include option which uses a type:

Screenshot 2022-07-21 at 14 55 10

Changing the PaymentEmbed from an enum to type fixes this:

Screenshot 2022-07-21 at 15 01 49 Screenshot 2022-07-21 at 15 03 16

Suggested Changes

Looking at the source code it seems like this change could be easy.

Lets use the PaymentStatus as an example.

The PaymentStatus enum from src/data/payments/data.ts:825 is used like an Object in src/data/payments/PaymentHelper.ts to check against the payment status.

I would suggest to change src/data/payments/data.ts from this:

export enum PaymentStatus {
  open = 'open',
  canceled = 'canceled',
  pending = 'pending',
  authorized = 'authorized',
  expired = 'expired',
  failed = 'failed',
  paid = 'paid',
}

To this:

export const PAYMENT_STATUS = {
  open: 'open',
  canceled: 'canceled',
  pending: 'pending',
  authorized: 'authorized',
  expired: 'expired',
  failed: 'failed',
  paid: 'paid',
};

export type PaymentStatus = keyof typeof PAYMENT_STATUS;

And then use it in src/data/payments/PaymentHelper.ts like this:

  public isOpen(this: PaymentData): boolean {
    return this.status === PAYMENT_STATUS.open;
  }

The PaymentStatus is now a type that can be used without problems.

Screenshot 2022-07-21 at 15 34 46

Conclusion

I don't know why you at Mollie use enums instead of types but looking at the code i don't see a good reason. Maybe i just didn't find it yet or maybe there is none. I just know that using the Mollie Node package would be better if they were types not enums.

If this change is welcome i would create a PR for that. But i would need some guidance regarding the API_KEY in the .env file (Just the regular API key or something special?) and regarding the amount of errors when running tsc --noEmit (Do i have to configure something special?). But I also do not insist on doing it myself. 😅

shwao commented 2 years ago

Oh damn. I just realized it... You are supped to use the Enum. I still think its cumbersome and my idea is better but at least i can use embed without any now. 🤦‍♂️

Pimm commented 2 years ago

Thanks for the issue.

You are ‒ as you found out on your own ‒ supposed to use the enum in your code when writing TypeScript:

mollieClient.payments.create({ method: PaymentMethod.ideal, … })

This makes code completion work, and eliminates the need for any.

However, I personally do not necessarily favour enums over union types of string constants (e.g. 'ideal' | 'creditcard' | 'directdebit' | …). Code completion works either way, and both are safe (in the sense of type safety). I also think in terms of readability or "code cleanness":

.create({ method: PaymentMethod.ideal, …

is not better or worse than:

.create({ method: 'ideal', …

(For the sake of completeness and to anyone reading this issue: if you're writing JavaScript, you can use either; the former is required when writing TypeScript.)

As I don't really have a personal preference, I decided against introducing new enums for new parts of the API. (You've stumbled upon this in the form of PaymentInclude.) I did so because I figured we can always replace union types for enums if we decide that that's the direction in which we want to go; whereas the reverse would cause a breaking change.

This is also the reason I haven't seriously considered de-enumming PaymentMethod: doing so would cause a breaking change.

I'll bump heads with some people about this and see what they have to say.

Regarding .env: for the time being, you'll have to put your API key in there if you want to run tests locally. Just go cp .env.example .env and fill it out.

Regarding the "amount of errors" you get when executing the TypeScript compiler, I would have to see those errors. I don't have enough information to help you at this point.

shwao commented 2 years ago

It just occurred to me while making coffee. (Pauses are great) I think i just haven't seen an example on https://docs.mollie.com/ that uses these enums and didn't think of it before.

Code completion works either way

Kind off. Union types work better and faster as you don't need to import anything for it. Just type a quote and all the possible values are displayed. Also hovering over a union type does show you the possible values while the enum just shows the name PaymentStatus.

Union type:

https://user-images.githubusercontent.com/63549997/180244193-6663903e-2f3b-4f83-bed8-4767a2be6bee.mov

Versus enum (ignore that Finger-Muscle-Memory-Error):

https://user-images.githubusercontent.com/63549997/180244571-e5a05298-033d-473e-b278-5346b1083ece.mov

Also i think in most IDE themes strings are better visible than variables which makes the union type better visible (at least for me).

I also realized that changing PaymentStatus from an enum to a type would require refactoring and that this could only happen in a major version.

Okay thanks for your answer, i I look forward to what your colleagues say. In the meantime ill just use those enums.