-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Increase tail-recursive conditional types from 50 to 1000 for variables #46
Conversation
Co-authored-by: HaiNNT <HaiNNT1302@gmail.com>
|
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.
The changes to the example aren't in scope of this PR 😅 we don't want your schema/bun to become the norm for our examples. It's also missing a changeset
@JoviDeCroock my bad, didn't mean to put our schema on the main repo. I've corrected it. Changing this PR to draft as we've also improve the same issue for fragments and results too. hold on! |
src/t.ts
Outdated
> | ||
: IntersectionAcc | UnionAcc; | ||
|
||
type getPossibleTypeSelectionRec< |
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.
This looks to be the build file can we remove this
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.
thanks, file removed
fix: improve getScalarTypeNames performance Co-authored-by: HaiNNT <HaiNNT1302@gmail.com>
@@ -221,11 +221,16 @@ type mapIntrospection< | |||
? Query['__schema']['subscriptionType']['name'] | |||
: never; | |||
types: mapIntrospectionTypes<Query, Scalars>; | |||
introspection: Query; |
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.
Why are we adding the full introspection here again?
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.
just to clarify why we're hesitant about introspection
and typesList
here. This makes our future plans for #10 complex.
The mapIntrospection<>
output isn't just an intermediary type to us, but a “format” that we wish to output directly (either in the LSP or in API/schema builders) in the future. So, adding more formats and variations to it will make things difficult.
@@ -221,11 +221,16 @@ type mapIntrospection< | |||
? Query['__schema']['subscriptionType']['name'] | |||
: never; | |||
types: mapIntrospectionTypes<Query, Scalars>; | |||
introspection: Query; | |||
typesList: Query['__schema']['types']; |
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.
This increases the memory footprint by quite a lot, can't we iterate over the values/keys instead of allocating an additional list
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.
For the current implementation of getScalarTypeNames
as below, It uses Schema['types'][keyof Schema['types']]
to get all the value types of Schema['types']
. This implementation causes a performance issue if there are too many keys. In the case of our Schema, we have thousands of keys (more than 15000 types) so the TS server crashed.
type getScalarTypeNames<Schema extends IntrospectionLikeType> =
Schema['types'][keyof Schema['types']] extends infer Type
? Type extends { kind: 'SCALAR' | 'ENUM'; name: any }
? Type['name']
: never
: never;
So we change the implementation, then it works.
Schema['typesList'][number]
has no performance issue with thousands of items.
type getScalarTypeNames<Schema extends IntrospectionLikeType> =
Schema['typesList'][number] extends infer Type
? Type extends {
kind: 'ENUM' | 'SCALAR';
name: any;
}
? Type['name']
: never
: never;
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.
I'm not convinced that's why the TSServer crashes.
Schema['types'][keyof Schema['types']]
is not necessarily the issue, but
Schema['types'][keyof Schema['types']] extends infer Type
is the problem because if we have n
keys, infer Type
becomes n
types and for each type we then filter them by name
, i.e. the problem is likely that the complexity of filtering is n x n
.
I'm not convinced though that passing on typesList
is the best solution 🤔
I do this rarely, so first I'll apologise 😅 The tl;dr is I'd like to close this PR for now, and address the concerns separately. I'm sorry, and I'm grateful for the time you've put in, but this will need more bisections and validations on our end. Regarding issue #47 in general, I'm happy to look into the performance of selecting enums. However, there's also other changes in this PR, all separate and there's no way for me to follow why they've been made and they compromise on clarity in a lot of place:
etc; basically, the PR doesn't document what's being addressed, why it's being addressed, and I also can't reproduce all of this. So, I feel, the best step forward for now here is for me to close this, note down the concerns in #47 and extract the concerns this PR is addressing, and follow along and reproduce this from scratch. If this then leads to new PRs — don't worry — we'll include you as a co-author, so you do get credit for the change. |
Type, | ||
Introspection, | ||
Fragments, | ||
UnionAccumulator |
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.
Ok, I think I know what confused me about this 😅 Sorry!
The UnionAccumulator
seems to be an attempt to make this tail-recursive. However, there's no direct recursion going on here.
So, it basically adds indirection, since it hoists this result too much.
Just to double-check this, I did benchmark this and the recursion in the other place does work (intersection), since it's isolated to one recursive type, but UnionAccumulator
doesn't move the benchmarks and doesn't seem to serve a purpose 🤔
@deathemperor @HaiNNT: I've extracted the easier tail-recursive fixes into here: #51 However, I haven't looked at Edit: #51 and #53 should be mostly reflecting the changes here (I opted for a simpler solution for I'll get that published. Please do let me know if that's working for you. And thank you again, and sorry again for closing the original PR here! ❤️ |
thank you @kitten for your very careful and detailed explanation. This PR indeed wasn't good enough to address the concerns it was trying to solve as the DX over the last week was terrible with all the issues this PR caused. And again much appreciated what you've improved in 1.2.1! However, the and type checking still costs 5-10s for changes to result in our schema (I've made the reproducible repo private, let me know if you need access). I can also be contacted via Discord deathemperor#7188. |
Summary
Hello again! As we use gql.tada in our production code, there's an issue arises with variables causing
Type instantiation is excessively deep and possibly infinite
. This is because Hasura's GraphQL variables enable infinite conditions in relationship.fix #47
Set of changes
Increase tail-recursive conditional types from 50 to 1000 microsoft/TypeScript#45711