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

Open
opened 2023-03-06 12:36:34 +00:00 by WofWca · 5 comments
Contributor

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:

if (this.paths.create === '') {
throw new Error('This model is not able to create data.')
}

getAll would throw if you were to call it on UserSettingsService:

export default class UserSettingsService extends AbstractService<IUserSettings> {
constructor() {
super({
update: '/user/settings/general',
})

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

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

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

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

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?
Author
Contributor

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

@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.
Sign in to join this conversation.
No Milestone
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: vikunja/vikunja#2018
No description provided.