chore: better service method types based on paths
object #3267
No reviewers
Labels
No Label
area/internal-code
changes requested
confirmed
dependencies
duplicate
good first issue
help wanted
hosting
invalid
kind/bug
kind/feature
question
wontfix
No Milestone
No project
No Assignees
4 Participants
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: vikunja/frontend#3267
Loading…
Reference in New Issue
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?
Addresses #3199 (but not completely).
Things to check:
undefined
instead of''
.paths
objects are top-level instead of per-instance. Although they're shallowly cloned inside ofAbstractService
, just in case.TotpService
) required manual adjustments.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!
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
Oh, that looks pretty cool. Although the article says
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?
@ -7,3 +7,3 @@
import type {Right} from '@/constants/rights'
interface Paths {
interface PathsAll {
Why not call it
AllPaths
?Idk, "the most important part at the start", like naming Vue components.
@ -6,0 +6,4 @@
get: '/user/settings/avatar',
update: '/user/settings/avatar',
create: '/user/settings/avatar/upload',
} as const
Why the
const .. as const
? Isn't that duplicated?Nope, otherwise it says that the type of
get
,getAll
,create
arestring
s, not string literals.I just merged ts 5.0 in #3254
Why could mutable values in this context be a problem for us?
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.
@ -14,6 +14,7 @@ interface Paths {
delete : string
reset?: string
}
type PathsGeneric = Partial<PathsAll>;
Since
PathsAll
is only used here couldn't we make all entries optional instead?@ -44,0 +47,4 @@
/**
* @example
* type T = ReplacerDictFromPath<'/teams/{teamId}/members/{username}'>;
* const t: T = {
Please don't use single character var names.
Come on, it's just an example. What would you call it?
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?
Hope "dict" is good, to align with the type name.
Perfect :)
Not sure how to do this, even with TS 5. Can the
super
call affect the type of the inheriting class?38728d6aa9
to88ebea4807
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.
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.88ebea4807
to058951f1fd
Well, the child class could also have a generic parameter, but I think it can only be inferred from its constructor invocation, not
super
.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 :)
🤗🤗 I wouldn't be able to work on the project without you people building the foundation.
hey @WofWca are you still interested in this?
In a sense that whether I personally need this changed - no, and not in the foreseeable future.