feat: allow experimental customization of execution algorithm

[yaacovCR]

See: #3163 (comment) below.

This PR has been reworked on the basis of that suggestion.

Depends on: #3184 and #3185

[yaacovCR]

It might make sense to export more/all of the functions within the default field executor so that a custom executor could call them as necessary with modifications, but I suppose one could argue that such a move would reveal too much internals... any thoughts?

[IvanGoncharov]

@yaacovCR Thanks for the PR 👍
Is it a blocker for you or we can postpone it for after the 16.0.0 release (ETA one month)?

I'm asking since it can overlap with our plan to add support for field-level middlewares.
So I need to better understand your use cases before merging this one.
So if possible can we postpone it to after 16.0.0?

[yaacovCR]

Do you mean #1516 ?

consensus there seems to be to implement middleware in user land.

What I am proposing here is to allow execution hooks to allow similar customization of execution pipeline in user land as well.

I don’t mind postponing merging, my general thought is to merge as fast as possible, with the caveat that you go slow enough to merge the correct design...

So just putting aside when to merge, let’s jumpstart the discussion right now in terms of this PR and that issue. What are the plans for middleware and does this PR complement those plans or conflict or both?

[yaacovCR]

A use case for gateway would be to skip the getArgumentValues because you don’t need to validate the arguments if you are forwarding to graphql subservice, same in terms of coercion portion of completion.

another use case would be to just add debug timings for execution, resolution + completion, ie resolution of the whole tree

[yaacovCR]

Another use case is to allow passing custom things into parent alter execution of child, ie if the parent has external graphql data, resolve that differently including user land version of graphql Java concept DataFetcherResult https://www.graphql-java.com/blog/deep-dive-data-fetcher-results/

I basically am working in graphql-tools on a similar type that works with defer/stream as well

[yaacovCR]

Some of this might be Nice to embed within schema object, but I ran into circular type dependency that schema generation code then needs to reference execution code.

so I stitched to just global option on execute/graphql functions

any ideas there?

[IvanGoncharov]

A use case for gateway would be to skip the getArgumentValues because you don’t need to validate the arguments if you are forwarding to graphql subservice, same in terms of coercion portion of completion.

@yaacovCR You need to do coercion of input args and validation of output in terms of query that gateway receives
otherwise, you will return incorrect locations inside errors.

another use case would be to just add debug timings for execution, resolution + completion, ie resolution of the whole tree

Yes, this is exactly the use case for middleware together with logging profiling, aborting long execution, etc.
So I'm totally for allowing people to wrap resolvers but I have doubts about allowing them to replace executeField.

Some of this might be Nice to embed within schema object, but I ran into circular type dependency that schema generation code then needs to reference execution code.

Can be a good solution, can you please explain what was the issue in more detail?

[yaacovCR]

1. The schema stitching gateway forwards the errors, and rewrites the path correctly, not sure how it deals with locations. Our correct locations part of the spec?

2. In terms of your doubts about middleware overlapping, I have yet to see an alternative proposal about said middleware, maybe I am looking in the wrong place, I agree that it may overlap, but it seems to me, as I wrote above, that this solution for execution tracks exactly with how middleware within user land for field resolution. What specifically are you proposing as an alternative and why is it the more optimal solution? I agree that it makes sense to delay merging if there is a conflict, but I am not clear what exactly is delaying the exploration of what the conflict is, and what would be the best path forward. The discussion can continue in parallel to the work on v16 and need not be delayed a month (!). Put differently, this change may conflict with a different approach for middleware, but it IS an approach for middleware, and as far as I can tell, the only as yet proposed approach... I am just puzzled here.

3. The GraphQLFieldExecutor type currently lives in execution code, but if it was part of the type system, it would have to be moved into schema definition code. The executor takes the execution context as an argument, but that type is defined in the execution package. My initial implementation used typescript interface merging to get around that, defined Execution Context as empty interface (or maybe with schema and fragments only?) within the definition code and then added the fields (or the rest of fields) within the execution file. That worked, but I am not sure if that is the best solution...

[yaacovCR]

The spec has a must in terms of error path, but just a should in terms of location. Not really clear on the difference for my purposes.

[IvanGoncharov]

Put differently, this change may conflict with a different approach for middleware, but it IS an approach for middleware, and as far as I can tell, the only as yet proposed approach... I am just puzzled here.

So idea is that I'm open to wrapping stuff like resolvers if it doesn't affect performance too much.
But I don't think replacing part of the execution algorithm is a way forward.
If we replace part of execution there are no guarantees that it will behave the same way as default executeField and locations is just one example of what can go wrong.
Also, allowing people to override internal parts of execution makes it significantly harder to do refactoring optimization and other similar stuff.
For example, we discussed switching to something like MaybePromise to make implementation easier but we can't trust that custom executeField will do that so we need to do the additional wrapping.
To put it short I'm against allowing to replace any of the algorithm provided in a spec, including parts of execute.
That said I totally agree that sometimes when you work on complex stuff like e.g. stitching you maybe need to do some stuff differently so totally for exposing execution algorithms as public API and allowing to build custom executer with reuse maximum of source code.
So I'm for allowing to compose execution algorithm in different ways to suit different needs federation, stitching, etc.
A good example of this is collectFields idea is to remove dependency on ExecutionContex and move it utilities folder.

Another possible approach would be to convert the executor functions into class and expose it as "unstable" API,
similar to how it's done for the parser:

graphql-js/src/language/parser.ts

Lines 171 to 179 in 9a04b4c

	* This class is exported only to assist people in implementing their own parsers
	* without duplicating too much code and should be used only as last resort for cases
	* such as experimental syntax or if certain features could not be contributed upstream.
	*
	* It is still part of the internal API and is versioned, so any changes to it are never
	* considered breaking changes. If you still need to support multiple versions of the
	* library, please use the `versionInfo` variable for version detection.
	*
	* @internal

That said I still don't fully understand the problem that you trying to solve with change.
So if you have time to explain it maybe it would be more effective to set up a quick call and brainstorm an alternative solution for it. If it works for you, please ping me in Slack and we can figure some time that works for both of us.

[yaacovCR]

So idea is that I'm open to wrapping stuff like resolvers if it doesn't affect performance too much.

Please elaborate on your potential alternative middleware approach that you alluded to above, here, or somewhere else. You wrote originally that one reason for postponing implementing this change is that it may conflict with that feature. But this is that feature, or at least one implementation of it. What potential alternative is it competing with? I think that discussion should take place on GitHub, either here or at #1516, rather than on slack.

But I don't think replacing part of the execution algorithm is a way forward.
If we replace part of execution there are no guarantees that it will behave the same way as default executeField and locations is just one example of what can go wrong.
Also, allowing people to override internal parts of execution makes it significantly harder to do refactoring optimization and other similar stuff.
For example, we discussed switching to something like MaybePromise to make implementation easier but we can't trust that custom executeField will do that so we need to do the additional wrapping.
To put it short I'm against allowing to replace any of the algorithm provided in a spec, including parts of execute.
That said I totally agree that sometimes when you work on complex stuff like e.g. stitching you maybe need to do some stuff differently so totally for exposing execution algorithms as public API and allowing to build custom executer with reuse maximum of source code.
So I'm for allowing to compose execution algorithm in different ways to suit different needs federation, stitching, etc.
A good example of this is collectFields idea is to remove dependency on ExecutionContex and move it utilities folder.

If you expose parts of it, but you don't allow users to specify their custom executor and use those parts, it is only half the battle.

Another possible approach would be to convert the executor functions into class and expose it as "unstable" API,
similar to how it's done for the parser:

graphql-js/src/language/parser.ts

Lines 171 to 179 in 9a04b4c

	* This class is exported only to assist people in implementing their own parsers
	* without duplicating too much code and should be used only as last resort for cases
	* such as experimental syntax or if certain features could not be contributed upstream.
	*
	* It is still part of the internal API and is versioned, so any changes to it are never
	* considered breaking changes. If you still need to support multiple versions of the
	* library, please use the `versionInfo` variable for version detection.
	*
	* @internal

This would be fine, although I don't see the difference in exposing a class that is experimental vs a set of functions.

That said I still don't fully understand the problem that you trying to solve with change.

So if you have time to explain it maybe it would be more effective to set up a quick call and brainstorm an alternative solution for it. If it works for you, please ping me in Slack and we can figure some time that works for both of us.

I listed several use cases above. I think the submission stands on its own, although I would be happy to discuss this issue and any other issue on Slack at any time of your convenience. I agree that if you implement custom executors incorrectly, they could break the spec -- the custom executor would be just as responsible as the main package code to implement the spec correctly. It seems a bit heavy handed to forbid users of this package to try their hand at that, however. Any useful experimentation that is open-source could then be submitted upstream, which would be my ideal.

[IvanGoncharov]

Please elaborate on your potential alternative middleware approach that you alluded to above, here, or somewhere else. You wrote originally that one reason for postponing implementing this change is that it may conflict with that feature. But this is that feature, or at least one implementation of it. What potential alternative is it competing with? I think that discussion should take place on GitHub, either here or at #1516, rather than on slack.

Don't have anything concrete right now, I'm just worried that instead of solving use cases of end-users we expose some internal functionality. I would rather put effort into adding proper middleware support instead of maintaining a set of "internal hooks" like executeField.
So idea was to postpone it until we switch main to experimental and start drafting proper middleware support.

If you expose parts of it, but you don't allow users to specify their custom executor and use those parts, it is only half the battle.

Agree, the core issue here is there are multiple libraries that need to alter execution in some way and many of those are pretty unique e.g. graphql-jit previously they used a bunch of "internal" APIs but now it looks like they resorted to copy-paste bunch of source code. The key factor here is that different libraries need to customize different parts of the execution algorith so to satisfy them we basically need to expose a number of "internal hooks" similar to executeField.

So I think it's better to focus on refactoring out useful bits and pieces like collectFields.
Just an analogy, exacting reusable parts are like making Lego bricks that other libraries can use but adding hooks is like doing Jenga.

A good example is graphql function (also it real one people actually asked me about adding hooks there), instead of allowing you to replace parser, validator, etc. you can just reuse those parts and write your own version of graphql that satisfy your needs. If we add all hooks that people wanted we would need to replace every function called with a hook and result in graphql having 3 additional args (to replace parse, validate, execute) and hard to read client code.
Instead, you can reuse parse, validate, and execute to write the same functionality and your source code would be way more explicit and readable comparing to using hooks.
I know graphql is an extreme case of super-simple function and execute is a way more complex function but it illustrates the general approach we take even if it slow and require more effort.

That said I think we need to have the possibility for people to do rapid prototyping and pushing boundaries of GraphQL so that why I suggest exposing the internals of execute as a class, more on this bellow 👇

This would be fine, although I don't see the difference in exposing a class that is experimental vs a set of functions.

You can override methods on the class and have other methods to call overridden method.
You can achieve almost the same as this PR:

class CustomExecutionContext extends ExecutionContext {
  construct(args: ExecutionArgs) {
    super(args);
    // ...
  }

  executeField(/* ... */) {
    // you can access super.executeField if you need
    // or write your own implemenation
  }
}

function customExecute(args: ExecutionArgs) {
  const executeContext = new ExecuteContext(args);
  return executeContext.executeOperation(args.operationName);
}

Would it work for your use case?
If so I planned to do similar refactoring, together a bunch of other small changes in execute as part of 16.0.0 so I will try to do it over weekends.

P.S. You are completely right, that we need to discuss this stuff publicly and not on Slack since there no document explaining architecture stuff like this one. I just find it easier to explain it verbally but totally agree that is not scalable and doesn't help other contributors.

[yaacovCR]

I’m not quite sure why a class appeals to you more, but as I said above, it’s the same for me, and that sounds absolutely perfect for my needs.

thank you so much!!!!

[yaacovCR]

@IvanGoncharov

refactored based on your suggestion above, this now depends on #3184 and #3185

[IvanGoncharov]

@yaacovCR I need to review both #3184 and #3185 but from a quick look, they look good in general.
However as described in my previous comments I'm against adding arguments to execute, subscribe, graphql, etc.
I also worked on similar changes in parallel and have partly done change there I move more stuff into the ExecutionContext class including extracting of operation.
I'm also ok to move executeSubscription into execute.js.
After those changes execute, subscribe, graphql, etc. will be just a think wrapper against the ExecuteContext class.
And if you want to implement an advanced use case that requires modifying execution logic it's ok to write these thin wrappers yourself.

[yaacovCR]

Closed in favor of #3185, which takes up the above suggestion.