Skip to content

Replace resolved types in lexicographic schema sort#2779

Closed
mattleff wants to merge 3 commits intographql:masterfrom
mattleff:lexicographic-sort-replace-resolved-types
Closed

Replace resolved types in lexicographic schema sort#2779
mattleff wants to merge 3 commits intographql:masterfrom
mattleff:lexicographic-sort-replace-resolved-types

Conversation

@mattleff
Copy link
Copy Markdown

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

Comment thread src/utilities/lexicographicSortSchema.js Outdated
Comment thread src/utilities/lexicographicSortSchema.js Outdated
@IvanGoncharov
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Author

mattleff commented Sep 1, 2020

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

@mattleff
Copy link
Copy Markdown
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
Copy Markdown
Member

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

@IvanGoncharov
Copy link
Copy Markdown
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
Copy Markdown
Member

@mattleff
Copy link
Copy Markdown
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