Improve typings of reverse navigation properties #1456
Conversation
Change-type: patch
| @@ -97,7 +97,7 @@ const getServiceModel = ({ | |||
| const { service } = await sdkInstance.models.application.get( | |||
| slugOrUuidOrId, | |||
| { | |||
| $select: 'service', | |||
| $select: 'id', | |||
There was a problem hiding this comment.
The typings always attempted to block selecting reverse navigation resources but didn't work quite right, with the fixes it now blocks the select attempt as expected
| @@ -87,14 +87,14 @@ export interface Organization { | |||
| is_using__billing_version: 'v1' | 'v2'; | |||
| logo_image: WebResource; | |||
|
|
|||
| application: ReverseNavigationResource<Application>; | |||
| application?: ReverseNavigationResource<Application>; | |||
There was a problem hiding this comment.
I made all the reverse navigation props optional as the improved typings now require that since they are usually omitted (ie whenever they aren't explicitly expanded)
| @@ -95,14 +102,14 @@ type ExpandedProperty< | |||
| : never; | |||
|
|
|||
| export type ExpandResultObject<T, Props extends keyof T> = { | |||
| [P in Props]: ExpandedProperty<T, P, object>; | |||
| [P in Props]-?: ExpandedProperty<T, P, object>; | |||
There was a problem hiding this comment.
Now that the reverse navigation properties are always optional this removes their optionality for when they are expanded
| }; | ||
|
|
||
| type ExpandResourceExpandObject< | ||
| T, | ||
| TResourceExpand extends ResourceExpand<T>, | ||
| > = { | ||
| [P in keyof TResourceExpand]: ExpandedProperty< | ||
| [P in keyof TResourceExpand]-?: ExpandedProperty< |
There was a problem hiding this comment.
Now that the reverse navigation properties are always optional this removes their optionality for when they are expanded
| @@ -3,11 +3,11 @@ export interface AnyObject { | |||
| } | |||
|
|
|||
| export type PropsOfType<T, P> = { | |||
| [K in keyof T]: T[K] extends P ? K : never; | |||
| [K in keyof T]-?: T[K] extends P ? K : never; | |||
There was a problem hiding this comment.
This fixes returning 'a' | 'b' | undefined when any of the props are optional
| }[keyof T]; | ||
|
|
||
| export type PropsAssignableWithType<T, P> = { | ||
| [K in keyof T]: P extends T[K] ? K : never; | ||
| [K in keyof T]-?: P extends T[K] ? K : never; |
There was a problem hiding this comment.
This fixes returning 'a' | 'b' | undefined when any of the props are optional
| @@ -46,7 +46,14 @@ export type InferAssociatedResourceType<T> = | |||
| export type SelectableProps<T> = | |||
| // This allows us to get proper results when T is any/AnyObject, otherwise this returned never | |||
| PropsOfType<T, ReverseNavigationResource<object>> extends StringKeyof<T> | |||
| ? StringKeyof<T> | |||
| ? StringKeyof<T> extends PropsOfType<T, ReverseNavigationResource<object>> | |||
There was a problem hiding this comment.
This is necessary to keep things working nicely now that PropsOfType no longer incorrectly returns 'a' | 'b' | undefined when there are optional properties, meaning that PropsOfType<T, ReverseNavigationResource<object>> extends StringKeyof<T> now matches (previously it failed due to the undefined)
| @@ -250,7 +250,7 @@ await (async () => { | |||
| options: { | |||
| $select: ['id', 'device_name'], | |||
| }, | |||
| }); | |||
| } as const); | |||
There was a problem hiding this comment.
I expect that this is going to be an intrusive change for TS consumers to do in a patch :(
Especially since I don't expect adding as const to be obvious to users.
I will try to find a workaround.
There was a problem hiding this comment.
Honestly I'm not sure it's worth trying to fix @thgreasi, I think the simple fact might be that in order to make these typings work without breaking changes just might not be possible since I think we might well hit an issue where it relies on bugs that force an any and it is not fixable without as const (or some brand new version of typescript or otherwise some change that is not actually backwards compatible. So overall I think it'd make more sense to just eat the major and switch to using the pinejs-client-js typings which have been used more extensively at this point and were built with the benefits of modern typescript from the beginning.
Aside from that I no longer need this PR to acheive my original aims of using the auto-generated typings since I was able to avoid hitting the bug whilst also improving typings
Change-type: patch
Resolves: #
HQ:
See:
Depends-on:
Change-type: major|minor|patch
Contributor checklist