Allow to globally define table/record mapping #4071
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I don't think the failing tests are relevant to this PR. Seem to be failing for everyone. |
artursvonda
force-pushed
the
feature/automatic-table-types
branch
from
ea2615f
to
567f94f
Compare
|
I added composite types as well since the interface for selecting, inserting and updating might and probably will be different (when inserting, not all fields are required). |
|
@artursvonda This feature looks very promising 👍 Thanks. |
lorefnon
approved these changes
Oct 26, 2020
|
This needs to be documented. @artursvonda could you please open a PR against https://github.com/knex/documentation? |
|
Sure, will do. Adding to Typescript section? |
|
Yes, please! |
|
While writing the documentation, I realised I didn't check one more use-case so I updated tests and code a bit to make composite types stricter. Since it applies only to composite type, this will not affect existing users but provide additional value to using composite types. I added info about that in the documentation. |
|
Documentation PR knex/documentation#292 |
kibertoad
approved these changes
Oct 28, 2020
|
This one's good to go? |
|
@lorefnon Could you please take a look if documentation for this change makes sense to you? |
|
@kibertoad Sorry for the delay here. Yes, the docs look good. Thanks for your contribution @artursvonda. |
|
Released in 0.21.10 |
|
Fails with TypeScript 4.0.5: |
kibertoad
added a commit
that referenced
this pull request
Nov 1, 2020
|
#4100 Pushed updated PR with fixes errors. Sorry for that. |
Pl217
added a commit
to UN-OCHA/hpc-api-core
that referenced
this pull request
Aug 23, 2024
In Knex `v0.21.12`, the type definitions changed significantly, which affected our custom model library Knex `v0.21.11` type definitions: https://github.com/knex/knex/blob/4cfb3ea17b8580d370f096807b7f8954a32a5f89/types/index.d.ts Knex `v0.21.12` type definitions: https://github.com/knex/knex/blob/90b145dc84b1281b985fa5cedd09b8cb362add9e/types/index.d.ts If we diff the type definitions, we can see that they have introduced the following generic types: ```ts export type CompositeTableType< TBase, TInsert = TBase, TUpdate = Partial<TInsert> > = { base: TBase; insert: TInsert; update: TUpdate; }; type TableInterfaceScope = keyof CompositeTableType<unknown>; type ResolveTableType< TCompositeTableType, TScope extends TableInterfaceScope = 'base' > = TCompositeTableType extends CompositeTableType<unknown> ? TCompositeTableType[TScope] : TCompositeTableType; ``` The changes were introduced in knex/knex#4071 which was released in `v0.21.10` and reverted in `v0.21.11`, then reintroduced in knex/knex#4100 and re-released in `v0.21.12`. The goal of the feature was to allow to globally define types for table columns and the feature is opt-in, as it falls back to old behavior if `Tables` interfaces is not extended and it supports all old use-cases. But, this new generic introduced a problem with our custom library, which was written around Knex. Namely, the definitions of methods started using `CompositeTableType` and `ResolveTableType`, so for example, `insert` function changed from ```ts insert<TResult2 = number[]>( data: DbRecordArr<TRecord> ): QueryBuilder<TRecord, TResult2>; ``` to ```ts insert<TResult2 = number[]>( data: TRecord extends CompositeTableType<unknown> ? ResolveTableType<TRecord, 'insert'> : DbRecordArr<TRecord> ): QueryBuilder<TRecord, TResult2>; ``` We can see the conditional type is now used, trying to match the shape of `CompositeTableType` and use old behavior if not matching. Since we're not using global table definitions, but defining our own library, the problem occurs where Typescript cannot infer that our custom types are not matching the shape of `CompositeTableType<unknown>`. The problem is that we're using a conditional type now, which can be illustrated if we change it to: ```ts insert<TResult2 = number[]>( data: TRecord extends CompositeTableType<unknown> ? DbRecordArr<TRecord> : DbRecordArr<TRecord> ): QueryBuilder<TRecord, TResult2>; ``` i.e. both "branches" of conditional type are same. But the error is still there. Since our custom types don't match the shape of `CompositeTableType`, the conditional type `ResolveTableType` should, in theory, resolve to the "else" branch (i.e., `TCompositeTableType`). But, the issue here is related to TypeScript's limitations when it comes to resolving conditional types with complex generic parameters. In our case, the problem arises because the type parameter `TCompositeTableType` is not known at the point where TypeScript needs to resolve the conditional type. Let's create a simplified example that mimics the situation: ```ts // Simplified version of Knex's types interface CompositeTableType<T> { base: T; insert: Partial<T>; update: Partial<T>; } type ResolveTableType< TCompositeTableType, TScope extends 'base' | 'insert' | 'update' = 'base' > = TCompositeTableType extends CompositeTableType<unknown> ? TCompositeTableType[TScope] : TCompositeTableType; // Simplified version of our model custom types interface FieldDefinition { required: Record<string, unknown>; optional: Record<string, unknown>; } type InstanceDataOf<F extends FieldDefinition> = F['required'] & Partial<F['optional']>; type UserDataOf<F extends FieldDefinition> = F['required'] & Partial<F['optional']>; function createModel<F extends FieldDefinition>() { return { create: async (data: UserDataOf<F>): Promise<InstanceDataOf<F>> => { // Simulating Knex's insert operation // We get an error: "Type `UserDataOf<F>` is not assignable to type // `ResolveTableType<InstanceDataOf<F>, "insert">`" const result: ResolveTableType<InstanceDataOf<F>, 'insert'> = data; return result; }, }; } // In concrete usage, Typescript can infer the type const model = createModel<{ required: { id: number; name: string }; optional: { email: string }; }>(); const result = model.create({ id: 1, name: 'John' }); ``` In this example, TypeScript cannot definitively resolve the `ResolveTableType` conditional type because: 1. The `InstanceDataOf<F>` type is not fully known at the point where `ResolveTableType` is used. 2. TypeScript can't guarantee that `InstanceDataOf<F>` will never satisfy `CompositeTableType<unknown>`, even though we know it won't. This limitation occurs because TypeScript evaluates conditional types with type parameters in a deferred manner. It doesn't immediately substitute the actual types, which would allow it to definitively resolve the condition. To work around this limitation, we need to provide explicit type annotations or use type assertions at certain points in our code, effectively telling TypeScript which branch of the conditional type to use. But, we need to resort to using `as any` type assertions, which are far from ideal, and should really be avoided, but there is no option for more type-safe type assertion here, because Knex doesn't export the needed types. Usage of `as any` assertion isn't a huge problem in this case, because we're not really relying on Knex type definitions, but we have a wrapper around Knex, and all the strong typing happens in this wrapper library. The strong typing when calling methods from Knex was a nice addition in overall type safety, but we're not losing much, besides the clarity which method override is being matched exactly, but that was little convoluted already, as Ctrl+Click (or F12 - Go To Definition) wasn't really leading to the exact override in the previous version either. Nevertheless, `as any` assertion was used here as a last resort, because there are no other viable alternatives.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
For most use-cases, this would allow to globally define table/record mapping globally once and get all the benefits of static typing without always specifying Records manually.
This is opt-in as it will fall back to old behaviour if Tables interfaces is not extended and it supports all old use-cases.
Opening as a draft for further discussion and improvements.