search

j-frost / billomat-ts

Billomat API implementation in TypeScript for use in Node.js
MIT License
6 stars 5 forks source link

Extendability of api class #5

Open fxmanu opened 3 years ago

fxmanu commented 3 years ago

@j-frost Did you already think about a possibility to make the api class extendable? i.e. I'd like to use the pdf api-method, to request a pdf. For a quick fix, I think about a raw-method, to use api-methods which are not implemented, yet. What do you think?

j-frost commented 3 years ago

I'm not sure I understand the question. When you say "the api class", do you mean BillomatResourceClient? I must admin this part of the code base is somewhat old, and I'm not sure I like it any more. This project lived as a sub directory of another project for quite a while until I decided that others might also benefit and published it here.

If I were to rewrite BillomatResourceClient I think I wouldn't make it a class these days. What purpose does this class serve? It holds nearly no state, and only for a very short period of time. I think I would rewrite it as a higher order function.

However in the interest of practicality I think it's fine to leave as is for now. I'd say I prefer composition over inheritance in this case; so I think we should add mixins to the "special" classes (think a getAs(format: string) mixin for invoices for instance).

fxmanu commented 3 years ago

Sorry, yes I mean the BillomatResourceClient class.

Regarding your suggestion with the special classes: I'm also not sure, if I get this right. If I see it correctly, there is no special class. All instances are created, using the BillomatResourceClient class.

So we could either start adding specific classes for each resource type (or whenever it's needed) or we could just add a raw-method to the BillomatResourceClient class, which could be more generic. I already implemented it and could create a draft pull request later.

What do you mean by a method like getAs(format: string). Was this meant as an example for getting the resource as pdf? If yes, in my opinion this would be too far from the billomat-API and I'd prefer method names like invoices.pdf for requests like GET /api/invoices/{id}/pdf and take the id as the first argument.

fxmanu commented 3 years ago

Furthermore, I don't really get what you mean by composition in this case? Could you please give a brief example on how you would implement this with the getAs(format: string)-mixin and a "special" class named Invoice.

Like this?

j-frost commented 3 years ago

When I wrote "special" classes, what I meant was that some functionality is specific to the Billomat resource. Some for instance may be GETed as PDFs, but others can't. This is exacerbated by the fact that there's even resources that don't offer all CRUD operations. For the first draft, I ignored this. I think one way to better capture the actual Billomat API is probably to have a base class that gets mixin style methods attached to it depending on what actual resource it's constructed for.

Providing a way to access the raw API may alleviate some of these issues in the short term, so I'm not opposed. It's tech debt though and I hope we'll pay it back some time :)

Regarding naming, I do think it helps to give functions and method verb names. So getAsPdf over pdf for instance.

The example mixins you posted are very much aligned with what I meant. I was gonna post an example here, but that blog really captures it well :)

j-frost commented 3 years ago

Alright so thanks to #6 we now have a raw method. Mixins are still todo though, so I'm leaving this issue open.