-
Notifications
You must be signed in to change notification settings - Fork 49
Ensure schemas can apply defaults when inserting #209
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.
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
Conversation
🦋 Changeset detectedLatest commit: 7b86a18 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@tanstack/db-example-react-todo @tanstack/db
@tanstack/electric-db-collection
@tanstack/query-db-collection
@tanstack/react-db
@tanstack/vue-db
commit: |
Hey it's looking good! A few failing tests to fix still... |
…into zod-schema-defaults
…into zod-schema-defaults
Hey I've fixed the previously failing tests but after merging in the new changes there's a bunch of different test failures. I'm moving houses tomorrow so I won't be able to address this for some time. If anyone has the moment to look into these conflicts I'd appreciate your help! Thanks |
👍 I'll pick it up after lunch |
@samwillis could you review my type changes? Particularly to the query builder? Don't want to mess with any guarantees you wanted. Our types are getting complicated... :-\ |
Thanks for picking this up. I have an idea that could simplify some of the collection types: export class CollectionImpl<
T extends object = Record<string, unknown>,
TKey extends string | number = string | number,
// Replace `TExplicit` `TSchema` `TFallback` with TInsertInput:
TInsertInput extends object = T,
> {
// ...
}
export class Collection<
T extends object = Record<string, unknown>,
TKey extends string | number = string | number,
TUtils extends UtilsRecord = {},
// Replace `TExplicit` `TSchema` `TFallback` with TInsertInput:
TInsertInput extends object = T,
> {
// ...
}
export function createCollection<
TExplicit = unknown,
TKey extends string | number = string | number,
TUtils extends UtilsRecord = {},
TSchema extends StandardSchemaV1 = StandardSchemaV1,
TFallback extends object = Record<string, unknown>,
>(options): Collection<
ResolveType<TExplicit, TSchema, TFallback>,
TKey,
TUtils,
// Pass the resolved type:
CollectionInsertInput<TExplicit, TSchema, TFallback>
> {
// ...
const collection = new CollectionImpl<
ResolveType<TExplicit, TSchema, TFallback>,
TKey
// Pass the resolved type:
CollectionInsertInput<TExplicit, TSchema, TFallback>
>(options)
return collection as Collection<
ResolveType<TExplicit, TSchema, TFallback>,
TKey,
TUtils
// Pass the resolved type:
CollectionInsertInput<TExplicit, TSchema, TFallback>
>
} This is similar to how ResolveType already works, only passing the resolved type instead of the 3 generics. |
|
||
// Minimal data | ||
const tx1 = createTransaction({ mutationFn }) | ||
tx1.mutate(() => collection.insert({ text: `task-1` })) |
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.
can you test asserting on tx.mutations[0] (changes, etc.)?
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.
also add some type tests in the collection.test-d.ts file
Hey @KyleAMathews I've written some tests but having trouble deciding what to do with the mutation types If we want full type safety on the All of these helpers: export type UpdateMutationFnParams<T extends object = Record<string, unknown>> =
{
transaction: TransactionWithMutations<T>
}
export type InsertMutationFnParams<T extends object = Record<string, unknown>> =
{
transaction: TransactionWithMutations<T>
}
export type DeleteMutationFnParams<T extends object = Record<string, unknown>> =
{
transaction: TransactionWithMutations<T>
}
export type InsertMutationFn<T extends object = Record<string, unknown>> = (
params: InsertMutationFnParams<T>
) => Promise<any>
export type UpdateMutationFn<T extends object = Record<string, unknown>> = (
params: UpdateMutationFnParams<T>
) => Promise<any>
export type DeleteMutationFn<T extends object = Record<string, unknown>> = (
params: DeleteMutationFnParams<T>
) => Promise<any>
// ...
/**
* Utility type for a Transaction with at least one mutation
* This is used internally by the Transaction.commit method
*/
export type TransactionWithMutations<
T extends object = Record<string, unknown>,
> = Transaction<T> & {
mutations: NonEmptyArray<PendingMutation<T>>
}
export interface TransactionConfig<T extends object = Record<string, unknown>> {
/** Unique identifier for the transaction */
id?: string
/* If the transaction should autocommit after a mutate call or should commit be called explicitly */
autoCommit?: boolean
mutationFn: MutationFn<T>
/** Custom metadata to associate with the transaction */
metadata?: Record<string, unknown>
}
(and more) All these helpers ultimately wrap around export interface PendingMutation<
T extends object = Record<string, unknown>,
TOperation extends OperationType = OperationType,
> {
// ...
original: TOperation extends `insert` ? {} : T
// The result state of the object after all mutations.
modified: T
// The actual changes made.
changes: TOperation extends `insert`
? T
: TOperation extends `delete`
? T
: Partial<T>
// without TInsertInput
collection: Collection<T, any>
//...
} So if you wanted to make the changes property be fully type safe you could do something like this: export type ResolveTransactionData<
T extends object = Record<string, unknown>,
TOperation extends OperationType = OperationType,
TInsertInput extends object = T,
> = TOperation extends `insert`
? TInsertInput
: TOperation extends `delete`
? T
: Partial<T>
export interface PendingMutation<
T extends object = Record<string, unknown>,
TOperation extends OperationType = OperationType,
TInsertInput extends object = T,
TCollection extends Collection<T, any, any, any, TInsertInput> = Collection<
T,
any,
any,
any,
TInsertInput
>,
> {
mutationId: string
// The state of the object before the mutation.
original: TOperation extends `insert` ? {} : T
// The result state of the object after all mutations.
modified: T
// The actual changes made.
changes: ResolveTransactionData<T, TOperation, TInsertInput>
collection: TCollection
//....
} But then the trouble is - you would have to pass many generics to all the mutation helpers. I tried to refactor following this pattern to minimize complexity: However, unlike On collection the complexity is lower and definitely worth it to make the operation params fully type safe. It could just be left as Not sure, it's up to you. I'll push my attempt for visibility but it's just a draft currently. Current state of the code:
You're welcome to pick it up from here if you like, or I can try something more later this week, my brain is pretty fried for a moment from reading all those types 😆 |
This might be beyond me 😅 @kevin-dp any thoughts? |
In general I think type complexity is worth it — I've spent more days than I care for working on typescript for this lib but we expect 100s of thousands of engineers to use this lib so even marginal improvements to type safety are useful. |
I've read through #207 and the comments in this PR. Regarding #207, i could fully understand the use case and agree with the solution. However, i've not played enough with it to fully understand the benefits of the fully type-safe changes as explained in #209 (comment). From the perspective of developers, what would be the benefits of this approach? i.e. can we look at some concrete examples with and without the type safety of changes, to see how impacting it is. I generally agree that the more type safe the better, but having to juggle around 3 type parameters everywhere instead of 1 makes the code a lot more complex, hard to understand and maintain. So it really depends how big the benefit will be for developers. |
I've been chatting this through with @KyleAMathews. Here are my thoughts:
Having precise types for user input is really important because e.g. when developers pass an argument to a function and the argument type checks, you want the function to work on that input. That's why However, the type of outputs is a different story. When the output of a function (or the I don't think the slightly improved typing for |
Thanks for your input @kevin-dp that's exactly what I was thinking, completely agree. I'll finish this up now 😄 |
@DawidWraga I merged in main — could you check it over to see if it looks correct still? |
Actually did another through go over and it looks good to go — please do check though @DawidWraga when you have a chance and PR anything I missed! |
Thanks for merging this in and fixing the last few tweaks. I've reviewed the PR and everything looks good. One technical decision I wanted to run by you is what data to pass into the The Question: What data should the insert mutation See items.forEach((item) => {
const validatedData = this.validateData(item, `insert`)
mutations.push({
original: {},
modified: validatedData,
// Option A: raw input
changes: item,
// Option B: validated input
changes: validatedData,
// Option C: validated fields, without default fields (current approach)
changes: Object.fromEntries(
Object.keys(item).map((k) => [k, validatedData[k]])
),
})
}) Example scenario: const schema = z.object({
title: z.string(),
dueDate: z.coerce.date().optional(),
completed: z.boolean().default(false)
})
taskCollection.insert({
title: "lorem",
dueDate: "2025-07-20" // string, not Date
} Options:
Rationale for C: Shows user intent (no defaults) with transformed values (for consistency with stored data) Case for A: Simpler, pure user input representation Let me know if you'd prefer a different approach - happy to adjust it, or feel free to tweak it directly if that's easier! |
Hmm good question. I think c still makes the most sense as "changes" are how the model was changed not the raw inputs. I'd find it odd if people wouldn't want the cleaned up data. If they didn't, they should probably change their schema? I'm not super confident or feel strongly about this. I suspect either way would be fine but keeping everything at the end state sounds good right now. |
Co-authored-by: Kyle Mathews <[email protected]>
see #207 (comment) for details