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

feat: increase fault tolerance to deep types error #483

Merged
merged 2 commits into from
May 16, 2023

Conversation

schusovskoy
Copy link
Contributor

Closes #481

Background: TS treats differently simple type alias with generic parameters and type alias which uses conditional type inside. In particular using a conditional type instead of a simple generic doesn't create a new level of nesting. This feature allows us to create deep types without encountering "Type instantiation is excessively deep and possibly infinite" error.

In this PR I added DrainOuterGeneric type which transforms original type to conditional, and therefore removes outer generic type wrapper and reduces instantiation depth. Next I replaced every Record with ShallowRecord (the same type but without wrapper). But the key change was to wrap in DrainOuterGeneric every "simple" generic type alias located in the return type of query builders methods.

With this relatively small changes we're now able to safely remove every $assertType calls in test/typings/test-d/with.test-d.ts.

Also as a side effect it improves performance of type checking by 13%.

Running npm run test:typings before changes.
Screenshot 2023-05-12 at 00 29 34

Running npm run test:typings after changes.
Screenshot 2023-05-12 at 00 32 53

@vercel
Copy link

vercel bot commented May 12, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
kysely ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 16, 2023 6:09am

@koskimas
Copy link
Member

koskimas commented May 13, 2023

This is amazing! I didn't believe something like this was possible. The only thing I'm wondering is are we trusting an obscure typescript implementation detail here that could change in the future? It's still better than the alternative though.

The tests still seem to fail. If you're able to make them work, this will definitely get merged.

@koskimas
Copy link
Member

koskimas commented May 13, 2023

The test error is only about not having a .js suffix in some imports.

[esmimports] invalid imports in file /home/runner/work/kysely/kysely/src/util/column-type.ts
[esmimports] import { DrainOuterGeneric } from './type-utils'

@igalklebanov igalklebanov added typescript Related to Typescript enhancement New feature or request labels May 13, 2023
@igalklebanov
Copy link
Member

Great stuff @schusovskoy! 💪

We need to make sure this doesn't break autocompletion - probably doesn't.

@schusovskoy
Copy link
Contributor Author

The only thing I'm wondering is are we trusting an obscure typescript implementation detail here that could change in the future?

I think that it's like conceptual difference between type alias and conditional type, where the last is some kind of "computation" in type system and therefore doesn't require to create intermediate type bindings.

Anyway they have an issue which promise to solve the root cause of the problem and some day we can safely remove this "hack".

Add DrainOuterGeneric type.
Replace Record type with ShallowRecord.
Apply DrainOuterGeneric to  complex types of query builder methods.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request typescript Related to Typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Investigate excessively deep types
3 participants