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

Increase tail-recursive conditional types from 50 to 1000 for variables #46

Closed
wants to merge 2 commits into from

Conversation

deathemperor
Copy link
Contributor

@deathemperor deathemperor commented Jan 30, 2024

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

Copy link

changeset-bot bot commented Jan 30, 2024

⚠️ No Changeset found

Latest commit: 5369167

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Member

@JoviDeCroock JoviDeCroock left a 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

@deathemperor
Copy link
Contributor Author

@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!

@deathemperor deathemperor marked this pull request as ready for review January 31, 2024 05:22
src/t.ts Outdated
>
: IntersectionAcc | UnionAcc;

type getPossibleTypeSelectionRec<
Copy link
Member

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

Copy link
Contributor Author

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 <[email protected]>
@@ -221,11 +221,16 @@ type mapIntrospection<
? Query['__schema']['subscriptionType']['name']
: never;
types: mapIntrospectionTypes<Query, Scalars>;
introspection: Query;
Copy link
Member

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?

Copy link
Member

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'];
Copy link
Member

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@JoviDeCroock

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;

Copy link
Member

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 🤔

@kitten
Copy link
Member

kitten commented Feb 2, 2024

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:

  • The introspection is passed through (duplication)
  • A types list is passed on now (duplication)
  • Additional generics that I haven't checked yet which aren't quite following the top-down patterns (at best, this requires some refactors, but I also am not sure why they're there)
  • There's a separate concern that's being addressed of input objects that are too big (but I don't think the fix lines up with how we're solving this in other places, and may not be effective)

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.
I know this doesn't sound very efficient, but it comes from an approach of “write-twice”, so we're on the same page here and reconcile this with the structure and code we already have two, while also getting a better idea of the performance issues you're seeing.

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.

@kitten kitten closed this Feb 2, 2024
Type,
Introspection,
Fragments,
UnionAccumulator
Copy link
Member

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 🤔

@kitten
Copy link
Member

kitten commented Feb 3, 2024

@deathemperor @HaiNNT: I've extracted the easier tail-recursive fixes into here: #51
I also found one additional case we missed before (getVariablesType)

However, I haven't looked at gql.scalar yet. I think, however, I'd want to go with a different approach, but this will take me a bit of time, since I have to get the testing cases for large schemas set up again to reproduce this.

Edit: #51 and #53 should be mostly reflecting the changes here (I opted for a simpler solution for graphql.scalar, since I think tsserver is always at risk of crashing with a big string literal union, regardless of how it's produced 🤔)

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! ❤️

@deathemperor
Copy link
Contributor Author

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 Type instantiation is excessively deep and possibly infinite.ts(2589) error still exists for type with deep graphql relations (our claim object).

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.

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.

Performance issue in getting enum with huge schema
4 participants