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

fix: $if inside with is { [x: string]: any }[]. #793

Merged
merged 1 commit into from
Dec 29, 2023

Conversation

igalklebanov
Copy link
Member

@igalklebanov igalklebanov commented Dec 4, 2023

closes #785.

This is voodoo, but works. Nothing else I tried did - the result of the inner $if was always { [x: string]: any }[].

Copy link

vercel bot commented Dec 4, 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 Dec 29, 2023 10:51am

@igalklebanov igalklebanov requested a review from koskimas December 4, 2023 00:17
@igalklebanov igalklebanov added bug Something isn't working typescript Related to Typescript labels Dec 4, 2023
@igalklebanov igalklebanov changed the title fix: $if inside with is any. fix: $if inside with is { [x: string]: any }. Dec 4, 2023
@igalklebanov igalklebanov changed the title fix: $if inside with is { [x: string]: any }. fix: $if inside with is { [x: string]: any }[]. Dec 4, 2023
@koskimas
Copy link
Member

koskimas commented Dec 4, 2023

I haven't told you this yet, but I spent a couple of days trying to understand the type performance issues in Kysely by profiling the typescript compiler. I found very serious issues like just checking if a SelectQueryBuilder A extends SelectQueryBuilder B takes about 100ms per check! It doesn't even matter how simple the DB interface is. It takes over 200ms to compare two ExpressionBuilder types. The comparisons are cached and each subsequent comparison of the exact same types takes no time. But each new variant takes 200ms, which is horrifying.

These type comparisons only get done if the user writes functions that take a specific type as an argument, or if the user uses explicit types. Most common Kysely code is not affected.

Each method in SelectQueryBuilder and ExpressionBuilder that return a new variant of SelectQueryBuilder or ExpressionBuilder make this much worse each time. For example removing the selectNoFrom method from ExpressionBuilder takes the comparison time from 200ms to 100ms. I think we need to remove selectNoFrom from ExpressionBuilder.

I need to setup a performance test suite so that we can keep track of this each time we add/change something. Including this PR.

I think we need to start seriously thinking what we can add to Kysely or it'll become unusable very soon.

@igalklebanov
Copy link
Member Author

Wow, great stuff. We sure need these benchmarks in place!

Was thinking about testing against multiple typescript versions too, at least the typings tests, so we're aware of what breaks for what version in latest 5 or so.

Copy link
Member

@koskimas koskimas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome! Checked this and it didn't slow things down.

@koskimas koskimas force-pushed the fix-if-inside-with-is-any branch from 2fa1979 to 10e282b Compare December 29, 2023 10:51
@koskimas koskimas merged commit 15ec87f into kysely-org:master Dec 29, 2023
5 checks passed
@igalklebanov igalklebanov deleted the fix-if-inside-with-is-any branch December 29, 2023 11:48
thecodrr pushed a commit to thecodrr/kysely that referenced this pull request Sep 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working typescript Related to Typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

WITH queries lose typesafety when using $if after select
2 participants