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

Replace resolved types in lexicographic schema sort #2779

Conversation

mattleff
Copy link

When using lexicographicSortSchema() with a schema containing resolveType for either interfaces or unions the resolved type wasn't being replaced with the new GraphQLObjectType created during sorting. This PR wraps any resolveType functions provided and substitutes the sorted named type for the type returned from the resolveType function.

Downstream issue: nestjs/graphql#1107

@IvanGoncharov
Copy link
Member

@mattleff Very interesting bug, once review comments are addressed I will merge it as a temporary fix.
But this uncovered generic issue since you can also reproduce with extendSchema or any other function that modify schema.
So the correct solution would be to just forbid returning GraphQLObjectType instance in the upcoming 16.0.0 so names of types would be the only option.
Do you see any potential problems with these approach?

@IvanGoncharov
Copy link
Member

@mattleff Thinking more about it it's better to have this temporary workaround directly inside execute:
https://github.com/graphql/graphql-js/blob/master/src/execution/execute.js#L1006-L1009
That way it's smaller and fix this problem for any schema transformation instead of just fixing lexicographicSortSchema

@mattleff
Copy link
Author

mattleff commented Sep 1, 2020

@IvanGoncharov Ok, I'll see if I can implement it in execute().

@mattleff
Copy link
Author

mattleff commented Sep 1, 2020

@IvanGoncharov I've pushed that change. Are you good with the current tests or is there a better place or method for testing this?

@IvanGoncharov
Copy link
Member

@mattleff Midnight in my location 🛏️
Will look into it tomorrow morning 🌅

@IvanGoncharov
Copy link
Member

@mattleff Took more time than I expected and resulted in #2790 #2791 #2793
So now we prepared for dropping GraphQLObject as a result of resolveType in the upcoming 16.0.0
Can you please test it with Next.js?

@IvanGoncharov
Copy link
Member

@mattleff You can use our NPM branch for tests:
https://github.com/graphql/graphql-js#want-to-ride-the-bleeding-edge

@mattleff
Copy link
Author

mattleff commented Sep 8, 2020

@IvanGoncharov That works great. Thanks for all your work to square this away!

@mattleff mattleff deleted the lexicographic-sort-replace-resolved-types branch September 8, 2020 14:40
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.

2 participants