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.

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

[types] Make Knex's default generic parameter types be assignable to knex's (TypeScript 4.6) #5021

Merged
merged 1 commit into from Mar 19, 2022

Conversation

ide
Copy link
Contributor

@ide ide commented Feb 12, 2022

The default generic parameters of Knex didn't match the generic parameter types of knex. This becomes relevant with TypeScript 4.6 for k: Knex = knex() to work.

We can't change knex's declaration from TResult = unknown[] to TResult = Record<string, any>[] because other types like DeferredKeySelection.ReplaceBase specifically look for unknown. Instead, this commit changes Knex to use TResult = any[].

This works with TypeScript 4.6. Added both tsd and dtslint tests (tsd doesn't support TS 4.6 yet). Also tested this change in a larger codebase and confirmed that nearly all TS 4.6 errors were addressed.

Follow-up to #4933.

types/index.d.ts Outdated Show resolved Hide resolved
@ide ide changed the title [types] Make knex's default generic parameter types match Knex's [types] Make knex's default generic parameter types match Knex's (TypeScript 4.6) Feb 12, 2022
@kibertoad
Copy link
Collaborator

Can you add a tsd test for this?

@ide ide changed the title [types] Make knex's default generic parameter types match Knex's (TypeScript 4.6) [types] Make Knex's default generic parameter types be assignable to knex's (TypeScript 4.6) Feb 12, 2022
@ide
Copy link
Contributor Author

ide commented Feb 12, 2022

When looking into tsd/dtslint tests, I ended up hitting a dead end since some of the higher-order types like DeferredKeySelection.ReplaceBase specifically look for unknown, so the original version of this PR that changed knex<..., TResult = unknown[]> to knex<..., Record<string, any>[]> ended up breaking other types.

Instead, the newest version of this PR changes Knex<..., TResult = Record<string, any>[]> to Knex<..., TResult = any[]>. This works with dtslint's copy of TypeScript 4.6 as well as tsc with a local copy of TypeScript 4.6. I was unable to test tsd with TypeScript 4.6 since it vendors its own fork of TS based on 4.5.x.

Furthermore I added some regression tests that fail with Knex<..., TResult = Record<string, any>[]> and pass with Knex<..., TResult = any[]>.

I understand if you don't want to take this PR since it makes the types looser.

…knex's

The default generic parameters of `Knex` didn't match the generic parameter types of `knex`. This becomes relevant with TypeScript 4.6 for `k: Knex = knex()` to work.

We can't change `knex`'s declaration from `TResult = unknown[]` to `TResult = Record<string, any>[]` because other types like `DeferredKeySelection.ReplaceBase` specifically look for `unknown`. Instead, this commit changes `Knex` to use `TResult = any[]`.

This works with TypeScript 4.6. Added both tsd and dtslint tests (tsd doesn't support TS 4.6 yet). Also tested this change in a larger codebase and confirmed that nearly all TS 4.6 errors were addressed.
@kibertoad
Copy link
Collaborator

@ide Do you think it's worth it to preserve unknown in higher-order types? Wonder if it brings value over Record.

@ide
Copy link
Contributor Author

ide commented Feb 12, 2022

I don't have a strong opinion on that since I don't directly use Knex's generic parameters (my data access layer above Knex is strictly typed, and the interface between this layer and Knex is covered by tests). My original motivation was to make k: Knex = knex({}) work with the TS 4.6 release candidate.

@ide
Copy link
Contributor Author

ide commented Mar 3, 2022

Revisiting this since TypeScript 4.6.2 has been released. I understand if you are on the fence and don't want to commit to taking this PR as the maintainer of the repo.

@kibertoad
Copy link
Collaborator

@OlivierCavadenti thoughts?

@kibertoad kibertoad merged commit 35e1963 into knex:master Mar 19, 2022
@KoltesDigital
Copy link

KoltesDigital commented Mar 23, 2022

Thanks for the PR, I was having the same problem and started a fork too before seeing this one.

I've introduced two types DefaultRecord and DefaultResult to unify these types across the typings file. I think this is still relevant. But I'm wondering what should these types be?

I don't know all API functions, but if TResult is supposed to always be an object, they should all be changed to Record<string, any>, instead of reverting this one back to any.

Besides, TypeScript doc encourages the use of unknown instead of any. This should not be a problem for users as they should override knex/types/tables. This could even encourage the ones which didn't override to do it. At least, it took a long time for me to find out the override ability, and maybe I would have found it sooner if the compiler was blocking me instead of letting me freely through.

Thoughts?

@ide ide deleted the @ide/knex-type branch March 23, 2022 17:43
@kibertoad
Copy link
Collaborator

Released in 1.0.5

lizthegrey added a commit to eve-val/eve-roster that referenced this pull request Apr 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants