refactor: services: add compile-time checks for existence of CRUD methods #3199

New Issue

WofWca · 2023-03-06T12:36:34Z

WofWca commented

2023-03-06 12:36:34 +00:00

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:

6aa02e29b1/src/services/abstractService.ts (L345-L347)

getAll would throw if you were to call it on UserSettingsService:
2b82df5dbd/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.

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`](https://kolaente.dev/vikunja/frontend/src/commit/6aa02e29b19f9f57620bdf09919df34c363e1f3d/src/services/abstractService.ts#L50) is shared for all the CRUD methods is weird to me. Explicitly listing supported query parameters would also be cool.

konrad commented

2023-03-06 18:57:21 +00:00

Do you have an idea on how to do this without much boilerplate?

WofWca commented

2023-03-06 19:04:45 +00:00

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.

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.

konrad commented

2023-03-06 19:07:11 +00:00

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.

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?

> 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. 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

2023-03-06 19:09:41 +00:00

What's preventing you to fit this in the class paradigm?

I kinda tried a bit, but then gave up. Should be possible.

> 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

2023-03-07 15:46:25 +00:00

@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.

@konrad Yes. We talked about this a while ago. I would love to have something like this: ```ts 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](https://github.com/ecyrbe/zodios#declare-your-api-with-zodios) 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](https://github.com/sindresorhus/ky). In v11 they want to allow this though.

👍 1

WofWca referenced this issue

2023-03-20 15:00:42 +00:00

chore: better service method types based on `paths` object #3267

Sign in to join this conversation.