Skip to content

Add Execution middleware #4

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

Closed
wants to merge 1 commit into from
Closed

Add Execution middleware #4

wants to merge 1 commit into from

Conversation

sahil-nyt
Copy link

@sahil-nyt sahil-nyt commented Mar 10, 2025

Allows a repo to use a chain-of-responsibility pattern to affect the execution behavior of graphql-go. Done for this use case: https://github.com/nytimes/scoop/pull/4863#discussion_r1987991818

@tilgovi
Copy link

tilgovi commented Mar 10, 2025

What can be done with this that cannot be done by wrapping the root operation resolvers? It seems like the use case described might be served by wrapping the root Query operation resolver, which receives all of the resolve parameters already parsed and prepared and such.

@tilgovi
Copy link

tilgovi commented Mar 10, 2025

I guess the way graphql-go structures things, it first determines the operation and then executes the fields. There's no resolver for Query or Mutation that ever gets called, so I'm a little off.

I could imagine deferring some things to the top-level field resolvers that we currently do in extensions, but I don't know if it's pretty. For example, the repo extension could try to defer acquisition of a connection to the field level, and somehow ensure that (through some context value) all fields acquire the same connection. Similar might work for single-flight. But maybe that's uglier than what you have here.

@chrisfrank
Copy link

This approach looks really nice to me, but to avoid diverging too much from upstream graphql-go, I think we should constrain our changes to either bug fixes or places where we're missing features from graphql-js. It looks like the graphql-js team considered and rejected an official middleware layer, so we should consider handling this in our app instead.

@sahil-nyt sahil-nyt closed this Mar 19, 2025
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.

3 participants