Further improve infer fields recursion #2452
Merged
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Okay: following upon the last PR (#2436), which successfully fixed the main error I was running into with adding fields containing circularity, I have by necessity been pushing this functionality through a much more rigorous use case, with a relatively large custom schema I am attaching to an object type in a current project.
As a result, I found two further errors downstream during conversion of large nested types, each of which has been added to the tests and then addressed in this PR:
after the PR, nested lists still weren't always protected for circularity correctly -- because, while @Zalastax echoed my thinking that
type.name
is always distinct, it turns out that the type.name key isn't always set yet at that point for hybrid (wrapped) types likeGraphQLList(OtherType)
. So, I went ahead and adopted the helpful prior suggestion of usingSet()
since it was cleared for compatibility in the discussion.the next error occurred when circularity protection ended up leaving a GraphQLObjectType with no child fields, when all were ignored due to circularity. I now test for types to have at least one accepted field before creation.
finally, I ran into one further apparent omission, which is the handling of the
GraphQLID
type. It is a scalar and an instance of GraphQLScalarType, but fell into the cracks on the list of types supported for filters, which led to an error if I switched the current recursion unit test to contain GraphQLID rather than GraphQLInt types at the deepest point. This type is now handled like the other scalars.After fixing all of those, my large, nested schema attaches without error via a plugin, and given the size of that schema, it should represent a near-comprehensive test of this functionality.
However, in a future PR, I'm very interested in pursuing an alternate API hook to
setFieldsOnGraphQLNodeType
(analogous to the existence of the-Statefully
alternative forcreatePages
). The use case is that there can be a great benefit to adding custom graphql sub-schemas into gatsby's schema via plugins, and some of those (like my current project) are built for querying an API outside of gatsby using various methods, particularly where that other API is far too large to simply import all nodes and let gatsby handle resolution, yet where there is still much to gain from allowing the data to be queried and pre-bundled at the page level with all the other graphql data. For these cases, it is generally not necessary for gatsby to automatically create a ton of filters to use down that long subtree, when those filters are often going to be unusable for these external APIs anyway. If there are thoughts on that alternative API hook or its naming, I'd be interested in putting it together.