Skip to content
This repository has been archived by the owner on Dec 8, 2021. It is now read-only.

Use GraphQLSchema to speed up schema parsing. #221

Merged
merged 1 commit into from
Nov 1, 2018

Conversation

stephenh
Copy link
Contributor

@stephenh stephenh commented Oct 30, 2018

Visiting DocumentNode directly was fairly tedious because there was
no map-based access to the types, so each time we wanted to look
up the type of a field/argument/etc., we had to start all over
at the top and rescan the AST (specifically extractTypeDefinition).

This adds a new GraphQLTypes to source-helper that is the trio
of types/unions/enums that is: a) built quickly via the graphql's
buildASTSchema, and b) built once up-front in parse.ts
and then replaces DocumentNode as what we pass around as our
current schema.

@stephenh stephenh force-pushed the use-graphql-tools branch 2 times, most recently from 8ee5369 to 717562d Compare October 30, 2018 20:12
@stephenh stephenh changed the title Use graphql-tools to speed up schema parsing. Use GraphQLSchema to speed up schema parsing. Oct 31, 2018
@stephenh
Copy link
Contributor Author

Updated to not pull in graphql-tools but instead use the same buildASTSchema primitive from graphql that the makeExecutableSchema was using.

@Weakky
Copy link
Contributor

Weakky commented Nov 1, 2018

Hey there 👋,

Thanks for this awesome PR. It looks like there are problems with GraphQLList and possibly with GraphQLNonNull types.

This is what the generated graphqlgen.ts file looks like on your PR:

image

Notice the => {}[] | Promise<{}[]> which is supposed to be of return type => Post[] | Promise<Post[]>

It looks like the GraphQLList types do not have any name property defined.

Would you mind fixing this issue, and making sure that the output of the yoga template is the same as before?

Thanks 🙏

@Weakky Weakky merged commit 5189447 into prisma-labs:master Nov 1, 2018
Weakky added a commit that referenced this pull request Nov 1, 2018
Weakky added a commit that referenced this pull request Nov 1, 2018
@Weakky
Copy link
Contributor

Weakky commented Nov 1, 2018

Bug should be fixed in ce962ab

Weakky added a commit that referenced this pull request Nov 1, 2018
@stephenh
Copy link
Contributor Author

stephenh commented Nov 1, 2018

Ha, nice! I'd just noticed the {} in our schema last night as well, and realized what you did, that a list + a non-null wasn't being traversed across the two hops to the final type. Thanks for the merge + clean up!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants