Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve typings of reverse navigation properties #1456

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Improve typings of reverse navigation properties #1456

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/models/organization-invite.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ const getOrganizationInviteModel = function (
name: roleName,
},
},
})
} as const)
: undefined,
]);
type OrganizationInviteBase = Omit<OrganizationInvite, 'invitee'>;
Expand Down
2 changes: 1 addition & 1 deletion src/models/service.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,7 @@ const getServiceModel = ({
const { service } = await sdkInstance.models.application.get(
slugOrUuidOrId,
{
$select: 'service',
$select: 'id',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

$expand: {
service: options,
},
Expand Down
116 changes: 58 additions & 58 deletions src/types/models.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,14 +87,14 @@ export interface Organization {
is_using__billing_version: 'v1' | 'v2';
logo_image: WebResource;

application: ReverseNavigationResource<Application>;
application?: ReverseNavigationResource<Application>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

/** includes__organization_membership */
organization_membership: ReverseNavigationResource<OrganizationMembership>;
owns__credit_bundle: ReverseNavigationResource<CreditBundle>;
owns__team: ReverseNavigationResource<Team>;
organization__has_private_access_to__device_type: ReverseNavigationResource<OrganizationPrivateDeviceTypeAccess>;
organization_credit_notification: ReverseNavigationResource<OrganizationCreditNotification>;
identity_provider_membership: ReverseNavigationResource<IdentityProviderMembership>;
organization_membership?: ReverseNavigationResource<OrganizationMembership>;
owns__credit_bundle?: ReverseNavigationResource<CreditBundle>;
owns__team?: ReverseNavigationResource<Team>;
organization__has_private_access_to__device_type?: ReverseNavigationResource<OrganizationPrivateDeviceTypeAccess>;
organization_credit_notification?: ReverseNavigationResource<OrganizationCreditNotification>;
identity_provider_membership?: ReverseNavigationResource<IdentityProviderMembership>;
}

export interface OrganizationCreditNotification {
Expand All @@ -113,9 +113,9 @@ export interface Team {
belongs_to__organization: NavigationResource<Organization>;

/** includes__user */
team_membership: ReverseNavigationResource<TeamMembership>;
team_membership?: ReverseNavigationResource<TeamMembership>;
/** grants_access_to__application */
team_application_access: ReverseNavigationResource<TeamApplicationAccess>;
team_application_access?: ReverseNavigationResource<TeamApplicationAccess>;
}

export interface RecoveryTwoFactor {
Expand All @@ -141,11 +141,11 @@ export interface User {
created_at: string;
username: string;

organization_membership: ReverseNavigationResource<OrganizationMembership>;
user_application_membership: ReverseNavigationResource<ApplicationMembership>;
team_membership: ReverseNavigationResource<TeamMembership>;
has_direct_access_to__application: ReverseNavigationResource<Application>;
user_profile: ReverseNavigationResource<UserProfile>;
organization_membership?: ReverseNavigationResource<OrganizationMembership>;
user_application_membership?: ReverseNavigationResource<ApplicationMembership>;
team_membership?: ReverseNavigationResource<TeamMembership>;
has_direct_access_to__application?: ReverseNavigationResource<Application>;
user_profile?: ReverseNavigationResource<UserProfile>;
}

export interface UserProfile {
Expand Down Expand Up @@ -180,7 +180,7 @@ export interface OrganizationMembership {
organization_membership_role: NavigationResource<OrganizationMembershipRole>;
effective_seat_role: string;

organization_membership_tag: ReverseNavigationResource<OrganizationMembershipTag>;
organization_membership_tag?: ReverseNavigationResource<OrganizationMembershipTag>;
}

export interface TeamMembership {
Expand Down Expand Up @@ -224,19 +224,19 @@ export interface Application {
organization: NavigationResource<Organization>;
should_be_running__release: OptionalNavigationResource<Release>;

application_config_variable: ReverseNavigationResource<ApplicationVariable>;
application_environment_variable: ReverseNavigationResource<ApplicationVariable>;
build_environment_variable: ReverseNavigationResource<BuildVariable>;
application_tag: ReverseNavigationResource<ApplicationTag>;
owns__device: ReverseNavigationResource<Device>;
owns__public_device: ReverseNavigationResource<PublicDevice>;
owns__release: ReverseNavigationResource<Release>;
service: ReverseNavigationResource<Service>;
is_depended_on_by__application: ReverseNavigationResource<Application>;
is_directly_accessible_by__user: ReverseNavigationResource<User>;
user_application_membership: ReverseNavigationResource<ApplicationMembership>;
team_application_access: ReverseNavigationResource<TeamApplicationAccess>;
can_use__application_as_host: ReverseNavigationResource<ApplicationHostedOnApplication>;
application_config_variable?: ReverseNavigationResource<ApplicationVariable>;
application_environment_variable?: ReverseNavigationResource<ApplicationVariable>;
build_environment_variable?: ReverseNavigationResource<BuildVariable>;
application_tag?: ReverseNavigationResource<ApplicationTag>;
owns__device?: ReverseNavigationResource<Device>;
owns__public_device?: ReverseNavigationResource<PublicDevice>;
owns__release?: ReverseNavigationResource<Release>;
service?: ReverseNavigationResource<Service>;
is_depended_on_by__application?: ReverseNavigationResource<Application>;
is_directly_accessible_by__user?: ReverseNavigationResource<User>;
user_application_membership?: ReverseNavigationResource<ApplicationMembership>;
team_application_access?: ReverseNavigationResource<TeamApplicationAccess>;
can_use__application_as_host?: ReverseNavigationResource<ApplicationHostedOnApplication>;
}

export interface UserHasDirectAccessToApplication {
Expand Down Expand Up @@ -379,14 +379,14 @@ export interface Release {
belongs_to__application: NavigationResource<Application>;

/** @deprecated Prefer using the Term Form "release_image" property */
contains__image: ReverseNavigationResource<ReleaseImage>;
release_image: ReverseNavigationResource<ReleaseImage>;
should_be_running_on__application: ReverseNavigationResource<Application>;
is_running_on__device: ReverseNavigationResource<Device>;
is_pinned_to__device: ReverseNavigationResource<Device>;
should_operate__device: ReverseNavigationResource<Device>;
should_manage__device: ReverseNavigationResource<Device>;
release_tag: ReverseNavigationResource<ReleaseTag>;
contains__image?: ReverseNavigationResource<ReleaseImage>;
release_image?: ReverseNavigationResource<ReleaseImage>;
should_be_running_on__application?: ReverseNavigationResource<Application>;
is_running_on__device?: ReverseNavigationResource<Device>;
is_pinned_to__device?: ReverseNavigationResource<Device>;
should_operate__device?: ReverseNavigationResource<Device>;
should_manage__device?: ReverseNavigationResource<Device>;
release_tag?: ReverseNavigationResource<ReleaseTag>;
}

export interface Device {
Expand Down Expand Up @@ -460,18 +460,18 @@ export interface Device {
/** This is a computed term that works like: `device.is_pinned_on__release ?? device.belongs_to__application[0].should_be_running__release` */
should_be_running__release: OptionalNavigationResource<Release>;

device_config_variable: ReverseNavigationResource<DeviceVariable>;
device_environment_variable: ReverseNavigationResource<DeviceVariable>;
device_tag: ReverseNavigationResource<DeviceTag>;
service_install: ReverseNavigationResource<ServiceInstall>;
image_install: ReverseNavigationResource<ImageInstall>;
device_config_variable?: ReverseNavigationResource<DeviceVariable>;
device_environment_variable?: ReverseNavigationResource<DeviceVariable>;
device_tag?: ReverseNavigationResource<DeviceTag>;
service_install?: ReverseNavigationResource<ServiceInstall>;
image_install?: ReverseNavigationResource<ImageInstall>;
}

export interface CpuArchitecture {
id: number;
slug: string;

is_supported_by__device_type: ReverseNavigationResource<CpuArchitecture>;
is_supported_by__device_type?: ReverseNavigationResource<CpuArchitecture>;
}

export interface DeviceType {
Expand All @@ -482,11 +482,11 @@ export interface DeviceType {
logo: string | null;
contract: Contract | null;
belongs_to__device_family: OptionalNavigationResource<DeviceFamily>;
is_default_for__application: ReverseNavigationResource<Application>;
is_default_for__application?: ReverseNavigationResource<Application>;
is_of__cpu_architecture: NavigationResource<CpuArchitecture>;
is_accessible_privately_by__organization: ReverseNavigationResource<Organization>;
describes__device: ReverseNavigationResource<Device>;
device_type_alias: ReverseNavigationResource<DeviceTypeAlias>;
is_accessible_privately_by__organization?: ReverseNavigationResource<Organization>;
describes__device?: ReverseNavigationResource<Device>;
device_type_alias?: ReverseNavigationResource<DeviceTypeAlias>;
}

export interface DeviceTypeAlias {
Expand Down Expand Up @@ -524,9 +524,9 @@ export interface Service {
created_at: string;
service_name: string;
application: NavigationResource<Application>;
is_built_by__image: ReverseNavigationResource<Image>;
service_environment_variable: ReverseNavigationResource<ServiceEnvironmentVariable>;
device_service_environment_variable: ReverseNavigationResource<DeviceServiceEnvironmentVariable>;
is_built_by__image?: ReverseNavigationResource<Image>;
service_environment_variable?: ReverseNavigationResource<ServiceEnvironmentVariable>;
device_service_environment_variable?: ReverseNavigationResource<DeviceServiceEnvironmentVariable>;
}

export interface IdentityProvider {
Expand All @@ -536,8 +536,8 @@ export interface IdentityProvider {
issuer: string;
certificate: string;
requires_signed_authn_response: boolean;
manages__saml_account: ReverseNavigationResource<SamlAccount>;
identity_provider_membership: ReverseNavigationResource<IdentityProviderMembership>;
manages__saml_account?: ReverseNavigationResource<SamlAccount>;
identity_provider_membership?: ReverseNavigationResource<IdentityProviderMembership>;
}

export interface SamlAccount {
Expand Down Expand Up @@ -571,7 +571,7 @@ export interface Image {
dockerfile: string;
error_message?: string | null;
is_a_build_of__service: NavigationResource<Service>;
release_image: ReverseNavigationResource<ReleaseImage>;
release_image?: ReverseNavigationResource<ReleaseImage>;
}

export interface ReleaseImage {
Expand Down Expand Up @@ -616,7 +616,7 @@ export interface ServiceInstall {
installs__service: NavigationResource<Service>;
application: NavigationResource<Application>;

device_service_environment_variable: ReverseNavigationResource<DeviceServiceEnvironmentVariable>;
device_service_environment_variable?: ReverseNavigationResource<DeviceServiceEnvironmentVariable>;
}

export interface EnvironmentVariableBase {
Expand Down Expand Up @@ -723,9 +723,9 @@ export interface Plan {
is_valid_from__date: string | null;
is_valid_until__date: string | null;

plan_feature: ReverseNavigationResource<PlanFeature>;
offers__plan_addon: ReverseNavigationResource<PlanAddon>;
plan__has__discount_code: ReverseNavigationResource<PlanDiscountCode>;
plan_feature?: ReverseNavigationResource<PlanFeature>;
offers__plan_addon?: ReverseNavigationResource<PlanAddon>;
plan__has__discount_code?: ReverseNavigationResource<PlanDiscountCode>;
}

export interface PlanAddon {
Expand Down Expand Up @@ -770,8 +770,8 @@ export interface Subscription {

is_for__organization: NavigationResource<Organization>;
is_for__plan: NavigationResource<Plan>;
subscription_addon_discount: ReverseNavigationResource<SubscriptionAddonDiscount>;
subscription_prepaid_addon: ReverseNavigationResource<SubscriptionPrepaidAddon>;
subscription_addon_discount?: ReverseNavigationResource<SubscriptionAddonDiscount>;
subscription_prepaid_addon?: ReverseNavigationResource<SubscriptionPrepaidAddon>;
}

export interface SubscriptionPrepaidAddon {
Expand Down
2 changes: 1 addition & 1 deletion typing_tests/pine-params.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ await (async () => {
options: {
$select: ['id', 'device_name'],
},
});
} as const);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


const checkUndefined: typeof result = undefined;
console.log(checkUndefined);
Expand Down
13 changes: 10 additions & 3 deletions typings/pinejs-client-core.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>>
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

? // If all of the properties match the reverse navigation resource, return all properties as we assume we're in an `any`/`AnyObject` case
StringKeyof<T>
: // Otherwise return only the properties that are not reverse navigation resources
Exclude<
StringKeyof<T>,
PropsOfType<T, ReverseNavigationResource<object>>
>
: Exclude<
StringKeyof<T>,
PropsOfType<T, ReverseNavigationResource<object>>
Expand Down Expand Up @@ -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>;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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<
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that the reverse navigation properties are always optional this removes their optionality for when they are expanded

T,
P extends keyof T ? P : never,
Exclude<TResourceExpand[P], undefined>
Expand Down
4 changes: 2 additions & 2 deletions typings/utils.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes returning 'a' | 'b' | undefined when any of the props are optional

}[keyof T];

// backwards compatible alternative for: Extract<keyof T, string>
Expand Down
Loading