| Summary: | Make APIClient be more useful | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Tomás Cohen Arazi (tcohen) <tomascohen> |
| Component: | Architecture, internals, and plumbing | Assignee: | Bugs List <koha-bugs> |
| Status: | NEW --- | QA Contact: | Testopia <testopia> |
| Severity: | normal | ||
| Priority: | P5 - low | CC: | dcook, jonathan.druart, kyle, martin.renvoize, matt.blenkinsop, me, paul.derscheid, pedro.amorim, tomascohen |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| Change sponsored?: | --- | Patch complexity: | --- |
| Documentation contact: | Documentation submission: | ||
| Text to go in the release notes: | Version(s) released in: | ||
| Circulation function: | |||
| Attachments: |
Bug 36369: Introduce an APIClientBase class
Bug 36369: Introduce an APIClientBase class |
||
I think this makes sense. Tomas are you able to provide a PoC we can use as a starting point to look at / discuss? Is there any reason not to build the javascript version of our OpenAPI spec and use that internally? (In reply to Pedro Amorim from comment #1) > I think this makes sense. > Tomas are you able to provide a PoC we can use as a starting point to look > at / discuss? Yes, it is my plan for this morning. (In reply to Kyle M Hall from comment #2) > Is there any reason not to build the javascript version of our OpenAPI spec > and use that internally? You mean for validating outgoing requests? (In reply to Tomás Cohen Arazi from comment #4) > (In reply to Kyle M Hall from comment #2) > > Is there any reason not to build the javascript version of our OpenAPI spec > > and use that internally? > > You mean for validating outgoing requests? I just did some tests and at least the clients generated by https://editor.swagger.io/ do not validate data. Their are other SDK generators out there but this one at least doesn't do it. If you want to validate input before passing it to api clients, please use zod or something similar. That makes it very declarative and easy to maintain. Here's a list of options compared by performance: https://moltar.github.io/typescript-runtime-type-benchmarks/ Created attachment 164201 [details] [review] Bug 36369: Introduce an APIClientBase class Sorry for the delay y'all. I didn't manage to make this work. But I think it highlights the idea of what I tried to propose. api-client-base.js could be implemented as an http-client.js tweak, but I wanted to make it self-contained to ease explaining it. And also, I prefer we use the already implemented method names instead of plain HTTP verbs. Happy to Meet/Jitsi/Zoom to talk about this if needed. Please take this as a constructive review of the already great toolset y've all built. Created attachment 164205 [details] [review] Bug 36369: Introduce an APIClientBase class I like it. It could work to reduce the number of lines in the -api-client.js.
Two points however:
1. The endpoint need to be configurable. 'this._baseURL + "/" + id' is too simple and does not work for everything (eg. /patrons/{patron_id}/holds, /trains/{train_id}/items/{item_id})
2. So far we have decided to provide what is implemented/used. With this we will offer in the view usage of routes that may not be implemented yet.
I have implemented something similar before. In this case we can't have simplicity and flexibility. The more flexible the api-client-base is, the more complex it will become. I think we should agree on the interface (and its quirks) first and then structure the base class around it. Maybe a meeting like Tomas suggested would be helpful. (In reply to Jonathan Druart from comment #11) > It could work to reduce the number of lines in the -api-client.js. > > Two points however: > 1. The endpoint need to be configurable. 'this._baseURL + "/" + id' is too > simple and does not work for everything (eg. /patrons/{patron_id}/holds, > /trains/{train_id}/items/{item_id}) In my opinion, such cases should be handled, maybe with some more parameters to api-client-base. I totally get it and saw it in the code. > 2. So far we have decided to provide what is implemented/used. With this we > will offer in the view usage of routes that may not be implemented yet. Am I right to interpret what you say like: 'if the Vue page doesn't allow deleting the resource, we shouldn't implement a delete() method'? It feels to me that this shouldn't be an issue... and would allow people to reuse this more? (In reply to Tomás Cohen Arazi from comment #13) > (In reply to Jonathan Druart from comment #11) > > 2. So far we have decided to provide what is implemented/used. With this we > > will offer in the view usage of routes that may not be implemented yet. > > Am I right to interpret what you say like: 'if the Vue page doesn't allow > deleting the resource, we shouldn't implement a delete() method'? > > It feels to me that this shouldn't be an issue... and would allow people to > reuse this more? Yes it is what I meant for "used". And I agree it is not really a problem. However it can be a problem for "implemented": If the endpoint does not exist in the controller should we provide it in the api-client? It does not make sense to me. (In reply to Jonathan Druart from comment #14) > (In reply to Tomás Cohen Arazi from comment #13) > > (In reply to Jonathan Druart from comment #11) > > > 2. So far we have decided to provide what is implemented/used. With this we > > > will offer in the view usage of routes that may not be implemented yet. > > > > Am I right to interpret what you say like: 'if the Vue page doesn't allow > > deleting the resource, we shouldn't implement a delete() method'? > > > > It feels to me that this shouldn't be an issue... and would allow people to > > reuse this more? > > Yes it is what I meant for "used". And I agree it is not really a problem. > However it can be a problem for "implemented": If the endpoint does not > exist in the controller should we provide it in the api-client? It does not > make sense to me. I think it is fair that a dev can try, but then get a 404. I would like to hear from others. |
I file this report for discussion. In my opinion, there's too much duplicated code on the API client libraries. And the APIClient "class" is just a shortcut to avoid instantiating the module-specific API clients: ```javascript import ERMAPIClient from "./erm-api-client"; ... export const APIClient = { erm: new ERMAPIClient(), ``` Then used everywhere like this: ```javascript import { APIClient } from "../../fetch/api-client.js" ... const client = APIClient.erm await client.agreements.count().then( ``` IMHO, we need a real APIClient class that would implement generic methods like: ```javascript create: params => this.post({ endpoint: params.endpoint ? params.endpoint : "/", body: params.resource, headers: params.headers }), ... getAll: params => this.get({ endpoint: params.endpoint ? params.endpoint : "/", params.query, headers: params.headers, }), ... count: params => this.count({ endpoint: params.endpoint ? params.endpoint : "/", "?" + new URLSearchParams({ _page: 1, _per_page: 1, ...(params.query && { q: JSON.stringify(params.query) }), }), }), ``` and the API Client classes (like ERMAPIClient) should inherit from it, somehow. For doing so, we will face another design roadblock: While I uderstand that ERMAPIClient is a single place to have all the ERM module-needed API actions, it still feels weird from an OOP perspective how things are organized. I feel like we should have individual classes, like: ```javascript export RecordSourcesAPIClient extends APIClient ``` and then have a new type of class along this lines: ```javascript import CitiesAPIClient from "./cities-api-client"; import RecordSourcesAPIClient from "./record-sources-api-client"; ... export const AdminAPIClient = { cities: new CitiesAPIClient(), record_sources: new RecordSourcesAPIClient(), ... }; ``` this way, we will avoid repeating ourselves (less bugs!) and the code is (I expect) more reusable. I also think the `embed` handling should just be a list for the different methods, like: embed: [ 'patron', 'patron.category' ] and generically handled by APIClient. No need to build the headers manually everywhere.