vikunja
/
frontend
Archived
6
39
Fork 49
Code Issues 1 Pull Requests 4 Projects Releases 26 Activity

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

Closed
WofWca wants to merge 1 commits from WofWca/frontend:type-safer-service-methods into main
pull from: WofWca/frontend:type-safer-service-methods
merge into: vikunja:main
Conversation 12 Commits 1 Files Changed 35 +336 -215
WofWca commented 2023-03-20 15:00:42 +00:00
Contributor
Copy Link

Addresses #3199 (but not completely).

Things to check:

  • Now paths default to undefined instead of ''.
  • paths objects are top-level instead of per-instance. Although they're shallowly cloned inside of AbstractService, just in case.
  • Most of the concrete services were modified with a regex, but some (like TotpService) required manual adjustments.
Addresses #3199 (but not completely). Things to check: * Now paths default to `undefined` instead of `''`. * `paths` objects are top-level instead of per-instance. Although they're shallowly cloned inside of `AbstractService`, just in case. * Most of the concrete services were modified with a regex, but some (like `TotpService`) required manual adjustments.
frederick commented 2023-03-20 15:04:31 +00:00
Member
Copy Link

Hi WofWca!

Thank you for creating a PR!

I've deployed the changes of this PR on a preview environment under this URL: https://3267-type-safer-service-methods--vikunja-frontend-preview.netlify.app

You can use this url to view the changes live and test them out.
You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/.

Have a nice day!

Beep boop, I'm a bot.

Hi WofWca! Thank you for creating a PR! I've deployed the changes of this PR on a preview environment under this URL: https://3267-type-safer-service-methods--vikunja-frontend-preview.netlify.app You can use this url to view the changes live and test them out. You will need to manually connect this to an api running somehwere. The easiest to use is https://try.vikunja.io/. Have a nice day! > Beep boop, I'm a bot.
dpschen commented 2023-03-20 15:37:32 +00:00
Member
Copy Link

Couldn't we use as const satisfies PathsGeneric to simplify the configuration?

EDIT:

Thought a bit more. Now unsure if the latter makes sense. I did find this new addition to ts though which seems to be exactly our usecase: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#const-type-parameters

Couldn't we use `as const satisfies PathsGeneric` to simplify the configuration? **EDIT:** Thought a bit more. Now unsure if the latter makes sense. I did find this new addition to ts though which seems to be exactly our usecase: https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#const-type-parameters
WofWca commented 2023-03-20 17:45:06 +00:00
Author
Contributor
Copy Link

new addition to ts

Oh, that looks pretty cool. Although the article says

Note that the const modifier doesn’t reject mutable values

Might want to play around and see what can be adjusted in the MR.

But I think the MR an improvement anyway. Do you think there is a reason to not merge it now?

> new addition to ts Oh, that looks pretty cool. Although the article says > Note that the const modifier doesn’t reject mutable values Might want to play around and see what can be adjusted in the MR. But I think the MR an improvement anyway. Do you think there is a reason to not merge it now?
konrad reviewed 2023-03-20 20:13:29 +00:00
src/services/abstractService.ts
@ -7,3 +7,3 @@
import type {Right} from '@/constants/rights'
interface Paths {
interface PathsAll {
konrad commented 2023-03-20 20:13:10 +00:00
Owner

Why not call it AllPaths?

Why not call it `AllPaths`?
WofWca commented 2023-03-21 07:47:35 +00:00
Author
Contributor

Idk, "the most important part at the start", like naming Vue components.

Idk, "the most important part at the start", like naming Vue components.
👍 1
konrad marked this conversation as resolved
src/services/avatar.ts
@ -6,0 +6,4 @@
get: '/user/settings/avatar',
update: '/user/settings/avatar',
create: '/user/settings/avatar/upload',
} as const
konrad commented 2023-03-20 20:12:25 +00:00
Owner

Why the const .. as const? Isn't that duplicated?

Why the `const .. as const`? Isn't that duplicated?
WofWca commented 2023-03-21 07:49:56 +00:00
Author
Contributor

Nope, otherwise it says that the type of get, getAll, create are strings, not string literals.

Nope, otherwise it says that the type of `get`, `getAll`, `create` are `string`s, not string literals.
👍 1
dpschen commented 2023-03-22 15:06:24 +00:00
Member
Copy Link

I just merged ts 5.0 in #3254

Oh, that looks pretty cool. Although the article says

Note that the const modifier doesn’t reject mutable values

Might want to play around and see what can be adjusted in the MR.

Why could mutable values in this context be a problem for us?

But I think the MR an improvement anyway. Do you think there is a reason to not merge it now?

Right now I prefer to reduce the amount of possible conflicts whereever possible. Because of that I would prefer not to define the paths as separate objects for now. Keep that RegEx though, we might get back to that later. Out of these reasons I would prefer it if we can solve the type problems using satisfies and const type parameters.

I just merged ts 5.0 in https://kolaente.dev/vikunja/frontend/pulls/3254 > Oh, that looks pretty cool. Although the article says > > > Note that the const modifier doesn’t reject mutable values > > Might want to play around and see what can be adjusted in the MR. Why could mutable values in this context be a problem for us? > But I think the MR an improvement anyway. Do you think there is a reason to not merge it now? Right now I prefer to reduce the amount of possible conflicts whereever possible. Because of that I would prefer not to define the paths as separate objects for now. Keep that RegEx though, we might get back to that later. Out of these reasons I would prefer it if we can solve the type problems using satisfies and const type parameters.
dpschen reviewed 2023-03-22 15:06:44 +00:00
src/services/abstractService.ts
@ -14,6 +14,7 @@ interface Paths {
delete : string
reset?: string
}
type PathsGeneric = Partial<PathsAll>;
dpschen commented 2023-03-20 15:24:57 +00:00
Member

Since PathsAll is only used here couldn't we make all entries optional instead?

Since `PathsAll` is only used here couldn't we make all entries optional instead?
dpschen reviewed 2023-03-22 15:08:33 +00:00
src/services/abstractService.ts
@ -44,0 +47,4 @@
/**
* @example
* type T = ReplacerDictFromPath<'/teams/{teamId}/members/{username}'>;
* const t: T = {
dpschen commented 2023-03-22 15:08:33 +00:00
Member

Please don't use single character var names.

Please don't use single character var names.
WofWca commented 2023-03-22 15:15:05 +00:00
Author
Contributor

Come on, it's just an example. What would you call it?

Come on, it's just an example. What would you call it?
dpschen commented 2023-03-22 15:27:47 +00:00
Member

I know it seems picky, but when I read it I had to think for a bit until I understood. So Especially because it's an example it should be clear. How about params?

I know it seems picky, but when I read it I had to think for a bit until I understood. So Especially because it's an example it should be clear. How about params?
WofWca commented 2023-03-22 15:37:12 +00:00
Author
Contributor

Hope "dict" is good, to align with the type name.

Hope "dict" is good, to align with the type name.
👍 1
dpschen commented 2023-03-22 15:38:46 +00:00
Member

Perfect :)

Perfect :)
dpschen marked this conversation as resolved
WofWca commented 2023-03-22 15:20:19 +00:00
Author
Contributor
Copy Link

Because of that I would prefer not to define the paths as separate objects for now

Not sure how to do this, even with TS 5. Can the super call affect the type of the inheriting class?

> Because of that I would prefer not to define the paths as separate objects for now Not sure how to do this, even with TS 5. Can the `super` call affect the type of the inheriting class?
WofWca force-pushed type-safer-service-methods from 38728d6aa9 to 88ebea4807 2023-03-22 15:33:54 +00:00 Compare
dpschen commented 2023-03-22 15:34:51 +00:00
Member
Copy Link

Not sure how to do this, even with TS 5. Can the super call affect the type of the inheriting class?

Without having tried it I assumed that https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#const-type-parameters is exactly made for such usecases.

> Not sure how to do this, even with TS 5. Can the `super` call affect the type of the inheriting class? Without having tried it I assumed that https://devblogs.microsoft.com/typescript/announcing-typescript-5-0-beta/#const-type-parameters is exactly made for such usecases.
WofWca commented 2023-03-22 16:07:50 +00:00
Author
Contributor
Copy Link

I tried for a bit. I don't think it's possible. If we extend a class with generic type parameters then it looks like we have to specify them right there in the class ... extends ... line. I don't think the type inference is gonna work like it does with functions where you don't have to specify the generic type explicitly.
https://stackoverflow.com/a/68737746/10406353

I don't think the addition of <const T extends ...> has to do with this.

I tried for a bit. I don't think it's possible. If we extend a class with generic type parameters then it looks like we have to specify them right there in the `class ... extends ...` line. I don't think the type inference is gonna work like it does with functions where you don't have to specify the generic type explicitly. https://stackoverflow.com/a/68737746/10406353 I don't think the addition of `<const T extends ...>` has to do with this.
WofWca force-pushed type-safer-service-methods from 88ebea4807 to 058951f1fd 2023-03-22 16:09:38 +00:00 Compare
WofWca commented 2023-03-23 08:08:08 +00:00
Author
Contributor
Copy Link

Well, the child class could also have a generic parameter, but I think it can only be inferred from its constructor invocation, not super.

Well, the child class could also have a generic parameter, but I think it can only be inferred from its constructor invocation, not `super`.
dpschen commented 2023-03-23 10:11:47 +00:00
Member
Copy Link

Okay thanks!

Before merging I would like try out some things but all your arguments sound convincing (mostly need to block some free time, where I can fully grasp the details ;) ).

Also I want to let you know that your help is really appreciated! I think I'm often bad in getting this accross :)

Okay thanks! Before merging I would like try out some things but all your arguments sound convincing (mostly need to block some free time, where I can fully grasp the details ;) ). Also I want to let you know that your help is really appreciated! I think I'm often bad in getting this accross :)
👍 1 ❤️ 1
dpschen requested review from dpschen 2023-03-23 10:11:56 +00:00
dpschen added the
changes requested
area/internal-code
 labels 2023-03-23 10:12:08 +00:00
WofWca commented 2023-03-23 10:19:28 +00:00
Author
Contributor
Copy Link

Also I want to let you know that your help is really appreciated! I think I'm often bad in getting this accross :)

🤗🤗 I wouldn't be able to work on the project without you people building the foundation.

> Also I want to let you know that your help is really appreciated! I think I'm often bad in getting this accross :) 🤗🤗 I wouldn't be able to work on the project without you people building the foundation.
❤️ 2 🎉 1
dpschen was assigned by konrad 2023-03-23 20:55:20 +00:00
konrad commented 2023-10-22 17:12:48 +00:00
Owner
Copy Link

hey @WofWca are you still interested in this?

hey @WofWca are you still interested in this?
WofWca commented 2023-10-23 08:10:38 +00:00
Author
Contributor
Copy Link

In a sense that whether I personally need this changed - no, and not in the foreseeable future.

In a sense that whether I _personally_ need this changed - no, and not in the foreseeable future.
konrad closed this pull request 2023-10-23 08:11:47 +00:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
dpschen
Labels
Clear labels   
area/internal-code

   
changes requested

   
confirmed

   
dependencies

   
duplicate

This issue or pull request already exists

  
good first issue

   
help wanted

Need some help

  
hosting

   
invalid

Something is wrong

  
kind/bug

Something is not working

  
kind/feature

New feature

  
question

More information is needed

  
wontfix

This won't be fixed

No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
dpschen
4 Participants
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/frontend#3267
No description provided.
Delete Branch "WofWca/frontend:type-safer-service-methods"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?