refactor: services: add compile-time checks for existence of CRUD methods
Original issue by WofWca on 2023-03-06T12:36:34.000Z
Right now every service has all the methods (getAll, create, post), but not all of them are actually implemented by all of the services. There is a runtime check:
https://kolaente.dev/vikunja/frontend/src/commit/6aa02e29b19f9f57620bdf09919df34c363e1f3d/src/services/abstractService.ts#L345-L347
getAll would throw if you were to call it on UserSettingsService:
https://kolaente.dev/vikunja/frontend/src/commit/2b82df5dbd1c8ea921de67842e78b6f9fad077af/src/services/userSettings.ts#L4-L8
would be nice if the property actually didn't exist instead. Or, IMO, better yet, if each CRUD method was exported separately somehow, for better modularity and ease to see what's used (and unused) where (yes I'm an OOP hater). Also the fact that loading is shared for all the CRUD methods is weird to me.
Explicitly listing supported query parameters would also be cool.
@kolaente commented on 2023-03-06T18:57:21.000Z:
Do you have an idea on how to do this without much boilerplate?
WofWca commented on 2023-03-06T19:04:45.000Z:
I didn't think too much about it, but I'm sure it shouldn't be exceptionally hard.
I was thinking about initially inferring the available methods from the Paths object provided to the constructor (e.g. if there's no getAll property then the getAll method should return never), but I haven't come up with a way to fit it in the class paradigm properly.
@kolaente commented on 2023-03-06T19:07:11.000Z:
I was thinking about initially inferring the available methods from the
Pathsobject provided to the constructor (e.g. if there's nogetAllproperty then thegetAllmethod should returnnever), but I haven't come up with a way to fit it in the class paradigm properly.
That sounds like it could work (can't say this 100% sure due to my lack of ts knowledge). What's preventing you to fit this in the class paradigm?
@dpschen Do you have any idea about this?
WofWca commented on 2023-03-06T19:09:41.000Z:
What's preventing you to fit this in the class paradigm?
I kinda tried a bit, but then gave up. Should be possible.
dpschen commented on 2023-03-07T15:46:25.000Z:
@konrad Yes. We talked about this a while ago.
I would love to have something like this:
const userApi = useRessource('https://jsonplaceholder.typicode.com',
({createEndpoint}) => ({
index: createEndpoint<User[]>('GET', 'users')
search: createEndpoint<User[]>('GET', 'users?:key=:value'),
create: createEndpoint<User>('POST', 'users'),
read: createEndpoint<User>('GET', 'users/:id'),
// note URL-only parameter - :id!
update: createEndpoint<User>('PUT', 'users/:id!'),
delete: createEndpoint<User>('DELETE', 'users/:id'),
}), [ log ])
This takes heavy inspiration from: https://stackblitz.com/edit/axios-actions-ts?devToolsHeight=100&file=src%2Flib.ts%2Csrc%2Fmain.ts
Zodiac does a good job. Sadly it's still axios based and I really would like to replace axios / make that fetcher independent. I would prefer using native fetch or something like ky. In v11 they want to allow this though.