Proposal: remove direct calls to resolve and resolveType

[rmhartog]

Apologies if this should be an RFC instead, but the suggested changes are not changing anything outside of Gatsby's internals.

Motivation

While attempting to implement an addition to the GraphQL schema of Gatsby (an @projection directive), I wrapped the internal schema with a transform, so that the functionality would work regardless of the underlying schema (on internal nodes, 3rd party schemas and schema customizations). While this modification of the schema did not change the behaviour of the schema (see 'How to reproduce' below), it still broke several tests, as well as the node-model, as both make assumptions about the resolve and resolveType functions, and call them directly.

While the tests pass in the current case, I think it is not unreasonable for resolvers to expect a 'well-behaved' GraphQL environment when executing. In particular, this expectation is broken when resolvers are called with an incomplete GraphQLResolveInfo object.

Proposal

In order to be future proof, I suggest the following changes. I have made experimental changes locally, and am willing to contribute these changes, if this direction is found to be acceptable.

Replace direct calls to GraphQL schema in tests

Replacing direct calls to internal GraphQL functions with an execution of a query against the schema that verifies the same behaviour.

• Replace calls to resolvers (field.resolve) in the tests with an actual query
• Replace calls to type resolvers (interface.resolveType or union.resolveType) to a query for __typename
• Replace assertions on the identity of resolvers with a query, a mock of that resolver, and an assertion that the mock was called

Replace resolveRecursive in the node-model with a query against the node

The node-model provides an interface to retrieve and query Gatsby's nodes through fields on the GraphQL schema, as well as an internal cache that reduces duplicate queries and speeds up filtering and caching. To do so, it currently recursively walks a list of fields and retrieves them by calling their field resolver, or getting the value directly from the node. This limits the types of fields and resolvers that can be added to nodes, as these calls are not in an actual GraphQL environment.

• Replace resolveRecursive with a query against the node constructed from fieldsToResolve

How to reproduce

There is a simple change that shows the places where the tests are 'brittle' against a non-destructive change to the schema.

Replace the following lines

gatsby/packages/gatsby/src/schema/schema.js

Lines 73 to 76 in a4cd7d5

	// const { printSchema } = require(`graphql`)
	const schema = schemaComposer.buildSchema()
	// console.log(printSchema(schema))
	return schema

and

gatsby/packages/gatsby/src/schema/schema.js

Line 112 in a4cd7d5

return schemaComposer.buildSchema()

with

const schema = schemaComposer.buildSchema()
return require(`@graphql-tools/wrap`).wrapSchema({ schema })

This is essentially a no-op, creating a schema that forwards all requests to the original. Running the tests with these changes breaks 82 tests in 7 test suites at the time of writing.

Closing remarks

The extensibility of Gatsby through the underlying GraphQL schema is great, and I tip my hat to anyone who contributed to this feature. I'd love the opportunity to contribute to this extensibility even further, and welcome any feedback to this proposal.

Even if wrapping the schema using wrapSchema or transforming the schema is not the direction Gatsby wants to move towards, I believe implementing the above will contribute to making the underlying system more robust and flexible.

Happy to hear your thoughts!

/cc @freiksenet (I ran into this when investigating #19953, after this change it is much easier to implement @proxy and @projection in a way that is agnostic of the underlying schema)

[vladar]

Hey @rmhartog !

The proposal sounds pretty reasonable - running a query instead of running resolveRecursive should work in theory. There are several factors that make me a bit skeptical though:

1. It may affect performance because we call this resolveRecursive for every single node of the type. Full GraphQL queries will add overhead which may be significant at scale. This can be addressed by benchmarks I guess.

2. This part of the codebase is one of the most complicated. We will likely miss some edge cases with this kind of change. So even if we decide to do this - we will likely postpone merging it till the next gatsby major version as it has a potential to break (sadly, no ETA on it at the moment).

3. We are thinking about alternatives to this materialization approach (exactly because it is a bit too complicated). It may be deprecated some day and then it doesn't make much sense to do this work. To be clear: we haven't decided anything yet, chances are high that nothing will change but this posibility still exists.

4. It is not entirely clear what value this change brings to gatsby users. I completely agree that things will be more logical and robust if we implement it this way but practical outcome of this for most users will likely be minimal (if not negative due to breaks in various edge cases).

But I may be missing something. I guess my point is that we need some practical justification for this refactor - what value it brings to the table? what features it unblocks?

I see your point about wrapping a schema but we won't be doing this in gatsby internals (gatsby chose schema customization approach instead). Unlocking wrapping by itself is a non-goal.

Those are just additional constraints and warnings for this proposal. If you are up to it, we can move forward!

And regardless of the outcome of this, thanks for suggesting! 💜 Before your proposal I didn't even think about it this way.

[rmhartog]

Hey @vladar,

thanks for taking the time to respond. Honestly, after looking at https://github.com/gatsbyjs/gatsby-graphql-toolkit, I myself started thinking a little bit differently about Nodes versus GraphQL queries.

The feature I was attempting to unblock is an @projection directive, that works on both the internal schema of gatsby, as well as 3rd party schema's and schema customizations. The goal for this directive is to extend the selection set that is passed through to the underlying schema when a specific field is queried. The current schema customization abilities are insufficient to make this change outside of gatsby's internals. It requires transforming the query before it is delegated to the underlying schema, which can not be implemented by custom resolvers.

I understand that unlocking wrapping in itself is a non-goal, but unlocking this functionality makes the implementation of such a directive much easier and less complex. Similarly, I noticed that the implementation of the @proxy directive is quite complicated and the impact is spread through several places of the codebase, as all resolvers should now understand the additional fields on the GraphQLResolveInfo, which are not part of the spec.

If, however, we fetch all fields we could ever need ahead of time, all of this becomes irrelevant. In that case, 3rd party schemas will not be exposed directly in gatsby's schema anymore.

Let me address the points you mentioned as well:

1. Fully agree that we should be wary of performance implications. My gut feeling is that the execution engine of graphql is not that much heavier than the recursive field traversal being done currently, but benchmarks might be needed to show that definitively.

2. I understand, and have invested some time already trying to understand all of the implications. Additional tests may very well be in order, and we'll need to carefully weigh the risk of each change. If that means it will be delayed until the next major in the end, that makes sense, but we should first consider what kind of edge cases we can face.

3. Indeed, the current approach is quite complicated. With the limited knowledge that I have, and as someone from outside the project, it seems like a lot of the complexity can be resolved or reduced by embracing GraphQL's model a bit more. I'm curious if these conversations are being held in a public forum? I'd love to follow along, if not contribute where I can. To your original point, I am aware of the risk that this might become redundant.

4. I hope that I've explained what kind of value we could unlock with this change. Clearly, this is a change to internals without direct value to the end-users. The goal is to unlock the ability to provide that value with future changes.

Finally, you mention

gatsby chose schema customization approach instead

but I don't believe the two approaches are at odds with each other. In particular, I wouldn't expose the option to wrap the schema to end-users or plugin developers, but nonetheless using this ability within gatsby internals allows for more flexible additions mentioned above. The change above can not be easily achieved using schema customization. I'm curious to hear if you disagree.

Again, thanks for your reply and looking forward to hearing your thoughts. Happy to hear any feedback, from you or from others interested in this topic.

[vladar]

A little bit more context on original plans for projection directive: #16091 (comment)

And also this: #4261 (comment)

@stefanprobst I am keen to hear your thoughts on this proposal when you have a chance.