-
Notifications
You must be signed in to change notification settings - Fork 26
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Support for PocketBase 0.23 with Improved expand Property Optionality Based on Texpand #107
base: main
Are you sure you want to change the base?
Conversation
Today, I've been thinking that the package needs better |
Thanks for this! Looks like a good improvement. Give me some time to go through and test it out. If you can check the integration tests too, that would be useful ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks again for the PR! Nice work, just a few questions/comments.
? T extends unknown | ||
? keyof T extends never | ||
? true | ||
: false | ||
: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these nested conditions necessary? I would keep it simple since it's really just checking for the default param
export const BASE_SYSTEM_FIELDS_DEFINITION = `// System fields | ||
export type BaseSystemFields<T = never> = { | ||
\tid: ${RECORD_ID_STRING_NAME} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
keep the /t
white space style, otherwise the output is inconsistent
@@ -16,7 +16,7 @@ export async function fromDatabase( | |||
const result = await db.all("SELECT * FROM _collections") | |||
return result.map((collection) => ({ | |||
...collection, | |||
fields: JSON.parse(collection.fields), | |||
fields: JSON.parse(collection.schema), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this need to change?
// Utility type to check if T is exactly unknown | ||
type IsExactlyUnknown<T> = | ||
unknown extends T | ||
? T extends unknown | ||
? keyof T extends never | ||
? true | ||
: false | ||
: false | ||
: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this just return the expected type and be joined with BaseSystemFields
? This would prevent some repetition and be more explicit about what it's doing.
// Utility type to check if T is exactly unknown | |
type IsExactlyUnknown<T> = | |
unknown extends T | |
? T extends unknown | |
? keyof T extends never | |
? true | |
: false | |
: false | |
: false | |
// Utility type to get expand field. If T is provided, expand is no longer optional | |
type ExpandType<T> = | |
unknown extends T | |
? T extends unknown | |
? { expand?: unknown } | |
: { expand: T } | |
: { expand: T } |
expand
Property Type Handling withTexpand
Generic ParameterThis update refines the type definitions for the expand property in
BaseSystemFields
and related types to make it optional when the generic parameterTexpand
is not specified (defaults to unknown), and required whenTexpand
is explicitly provided. Previously, the expand property’s optionality did not depend on whetherTexpand
was specified, which could lead to less precise type safety.Changes Made:
TExpand
tounknown
:BaseSystemFields<Texpand = unknown>
, the default type forTexpand
is nowunknown
instead ofnever
. This aligns with common TypeScript practices and ensures that whenTexpand
is not provided, it defaults to unknown.IsExactlyUnknown<T>
to accurately determine if a typeT
is exactlyunknown
. This utility is crucial because simply checkingT extends unknown
isn’t sufficient (as all types extendunknown
). The utility uses advanced conditional types to make this determination.BaseSystemFields
to use theIsExactlyUnknown
utility type in a conditional type that determines the optionality of the expand property.AuthSystemFields
andUsersResponse
to align with the newBaseSystemFields
definition by defaultingTexpand
tounknown
.Reasons for Changes:
expand
:Impact of Changes:
Examples:
Texpand
is omitted (defaults tounknown
):Texpand
is specified: