Skip to content
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

Expose query's selected fields to resolvers via context #428

Closed
wants to merge 4 commits into from

Conversation

keithmattix
Copy link

I submitted #373 a while back, but I will be closing it in favor of this PR which is off of my fork's master branch

@keithmattix keithmattix changed the title Expose query's selected fields to resolvers via context (rebased) Expose query's selected fields to resolvers via context Jan 2, 2021
@aarondl0
Copy link

Thank you for this. I built on this work to also expose the mutation arguments. This is really useful for mutations in Go with this library since you can't tell what fields are null vs completely omitted when binding to a struct (and map binding isn't supported by this library).

ProlificLabs#1

@pavelnikolov
Copy link
Member

@aarondl0 there is a different approach to that in #430

@@ -76,7 +76,7 @@ When using `UseFieldResolvers` schema option, a struct field will be used *only*

The method has up to two arguments:

- Optional `context.Context` argument.
- Optional `context.Context` argument. If the graphql query had nested subfields, then use the `SelctedFieldsFromContext(ctx context.Context)` getter method

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo here: Selcted -> Selected

@ventsislav-georgiev
Copy link

ventsislav-georgiev commented Jul 16, 2021

@pavelnikolov can one of these PRs get merged? Any approach of implementing the resolver's access to the requested fields will be sufficient for the users of the lib. It is a relatively small change and there are 4 open PRs for it. The first one dates back to 2017.

@mehran-prs
Copy link

Hi all,

This PR is great, I have been checked graphql documents and implementation in js and I was thinking about a third param(instead of context) that provides info to the resolver.
Just like what graphql-js does:

// See below about resolver functions.
type GraphQLFieldResolveFn = (
  source?: any,
  args?: {[argName: string]: any},
  context?: any,
  info?: GraphQLResolveInfo
) => any
 
type GraphQLResolveInfo = {
  fieldName: string,
  fieldNodes: Array<Field>,
  returnType: GraphQLOutputType,
  parentType: GraphQLCompositeType,
  schema: GraphQLSchema,
  fragments: { [fragmentName: string]: FragmentDefinition },
  rootValue: any,
  operation: OperationDefinition,
  variableValues: { [variableName: string]: any },
}

Then we can provide a helper method to returns selections from the GraphQLResolveInfo.
I think this could be just like what graphql documented.

Users had this problem in js too, and resolved it by the info param.

@keithmattix
Copy link
Author

@mehran-prs There are essentially 4 different PRs open for this feature with 4 different designs. You're not the only one who would like to see this option exposed as a third argument, but it's really up to the maintainers to pick which design they'd like to merge

@mehran-prs
Copy link

Oh, I see,
Thanks, @keithmattix

@pavelnikolov
Copy link
Member

I am aware that this has been the most requested feature for this library.
@mehran-prs thank you for sharing this - it is similar to what I want for this library. I don't want the selected fields in the context. That is the reason why it hasn't been merged yet. Yes, most probably there will be a third parameter with the selected fields and their arguments.
I will try to focus on this next week. In the mean time PRs adding this a 3rd param are welcome.
Thank you for your patience!

@pavelnikolov
Copy link
Member

@ventsislav-georgiev I know that it is a small change, however, this PR is not complete. I've been collecting requirements and apparently we need the field arguments too because they are needed for filtering data, for example, for paging. You are correct that people have been waiting a long time for this feature. However, I want us to get it right because once people start using it then it is difficult to upgrade it. Thank you for your patience and I will try to add it soon.

@keithmattix
Copy link
Author

In light of @pavelnikolov's direction here, I feel like it would be best for me to close this PR in order to filter out a lot of the noise going on with this feature request. There are at least a couple of other PRs that take the argument approach, and I feel that the community would be better served by iterating on those approaches instead mine. Godspeed in your decision-making @pavelnikolov, and thank you for your hard work maintaining this library.

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.

6 participants