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, schodkowy.omegi-0r, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | 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 |
Description
Tomás Cohen Arazi (tcohen)
2024-03-20 14:20:05 UTC
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. (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. Maybe we can just code an override for not implemented verbs/endpoints that throws an exception. So devs don't get confused? I'm willing to be back to this work/discussion. Imho if you want to re-do an API client, it would make a lot of sense to do it in the most modern/universal and proper way possible. So something that could be used standalone also outside of the Koha UIs, maybe even on a separate repo and published on NPM (both to allow external JS programs to interact with Koha, but to also ensure it's written in an objective manner, with no hacks or short-circuits that'd unnecessarily bind it inherently to the Koha UI?). Then it also begs to finally use TypeScript, as type-safe API client is a very good thing, and you have OpenAPI specs that define everything about the APIs. So not doing that would be a big waste. The only problem is that a project like https://openapi-ts.dev/ for automatically translating OpenAPI specs to TypeScript types requires OpenAPI 3.x in current versions, but Koha still didn't migrate from 2.x (would be a good opportunity to do it). Of course the JS parts could still use it after building and not be too concerned about it. But in a proper setup, even then, a proper IDE setup could still give developers the type hints in their IntelliSense, which'd still be an easy win. So we should use something like that. Another win of using automated OpenAPI->TypeScript type generation would be that new endpoints defined in the schema could already be available for usage without manually needing to define them in yet another place. And without needing to manually type out all the code to handle the new endpoints (which would be pretty nice if we want to accelerate the API development and phase out the old one and classic Perl scripting) There's two projects: 1. https://openapi-ts.dev/ 2. https://heyapi.dev/ It seems that #1 focuses on more "bare" but usable schema generation, which can be used with their openapi-fetch client to access APIs in a type-safe but close to the original specs manner. For example (mind you that TS detects the exact path used by strings and type-checks the parameters!!): const { data, error } = await client.GET("/blogposts/{post_id}", { params: { path: { post_id: "my-post" }, query: { version: 2 }, }, }); It seems that #2 generates more of "prettier" SDKs already and focuses on direct integrations with plugins and middlewares etc. Their example is more like: const { data, error } = await getPetById({ path: { // random id 1-10 petId: BigInt(Math.floor(Math.random() * (10 - 1 + 1) + 1)), }, }); Not sure which approach seems nicer. Also technically it could be that simply generated types lived on a separate repo generated for all versions (including point-releases or major version tags), with clients being more responsible with how they'd directly pull/integrate them? I think everything really depends whether people would prefer using something like in #1 and maybe writing some more functional simpler wrappers around some of them (such as `blogposts.getPostById(id: number)`, or directly use something as in #2 which is something in between. Also the #2 having integrations with something like Zod or TanStack Query could prove quite useful in the future developments with Vue components (validating and auto-fetching of stuff etc.). I mean just look at the example here: https://tanstack.com/query/latest/docs/framework/vue/quick-start It would remain to be checked which one would work better with paginated endpoints though. It seems the #2's TanStack Query's integration has some support for infinite queries there in that regard. |